Repository: elixir-ecto/ecto
Pull request #2249 (2017-09-28 14:00:31)

Add unsafe_fragment/1 query API
This PR adds support for unsafe_fragment/1 query API.

This is meant to be used to insert fragments only from interpolated binaries, to handle cases that fragment/1 is not able to handle.

This API may open the app to injection attacks if not used wisely, but proper warnings are in the doc and errors.

ebert[bot]: Ebert has finished reviewing this Pull Request and has found:

* 1 possible new issue (including those that may have been commented here).

You can see more details about this review at https://ebertapp.io/github/elixir-ecto/ecto/pulls/2249.

<!-- Ebert auto generated comment -->

OvermindDL1: If anything, I'd vote for a warning to be raised on every-single-compile to encourage users to get rid of it and/or get a proper API in.

However, on the other side, maybe put that warning behind an option that defaults to true or so, something like this would be very very useful in using SQL features that Ecto currently fails on, like multi-prefix joins or UNION's or etc...
OvermindDL1: A few things, you need to add tests first of all, and are you able to, say, put a subquery into the unsafe_fragment for things like UNION's and so forth?
xadhoom: Well,

having a warning *always* can be annoying because is meant to be used when the current API is not sufficient and encourage switching makes no sense is simply because not possible.

I agree that adding a "switchable" flag could restrain newbies to use it while giving others the possibility to shut it off. If so, is worth the effort? What other thinks?

About tests, you mean integration tests?
Query building tests are already present.
xadhoom: About using unsafe_fragments/1 for UNIONS and so on, I did not tested it and just followed how fragment/1 is tested.

Btw, unsafe_fragment is just the same AST as fragment without Keyword checks at runtime, while forcing only interpolated values at compile time and being a binary at runtime.
So the change is really minimal and where fragment/1 works also unsafe_fragment/1 will.


xadhoom: Another example where this may be useful:

https://elixirforum.com/t/querying-json-fields-with-dynamic-document-schema/7740/2
OvermindDL1: > https://elixirforum.com/t/querying-json-fields-with-dynamic-document-schema/7740/2

In that specific example from that post why not just:
```elixir
  defmacrop ilike_jsonb_fragment(query, field, [_ | _] = keys, value) do
    frag =
      keys
      |> Enum.map(fn _ -> "?" end)
      |> Enum.join("->")
      |> case do f -> "#{f}->>?" end
    keys = Enum.map(keys, &quote(do: ^unquote(&1))
    quote do
      from q in query,
        where: fragment(unquote(frag), field(q, ^String.to_existing_atom(field)), unquote_splicing(keys), ^value)
    end
  end
```
Or something like that?  You could then use it like:
```elixir
query = ilike_jsonb_fragment(query, "some_field", [key1, key2, key3], 42)
```
Which does not seem to require an unsafe_fragment at all.  I'm really not for adding `unsafe_fragment` unless it provides a clear benefit for something that is otherwise impossible in Ecto as it is, and yes it does have some use-cases, such as UNIONs and cross-schema joins and other such things, and tests for those kind of things really should be tested for, not for things that are already possible.
xadhoom: Well,

in the forum link, the problem was that the depth of the json document queried was not know at compile time.

I'm not a macro expert at all, but your example works if the keys are known at compile time, not runtime.
Or I'm mistaken?

Btw yes, I don't want to contradict on a more proper example you I'm just trying to learn a bit more from your answers :smile: 

Said that, what do you think for a more proper example to put into the docs ?
OvermindDL1: > I'm not a macro expert at all, but your example works if the keys are known at compile time, not runtime.
Or I'm mistaken?

The 'count' of keys have to be known at compile-time (you can branch on this function easily in that case), but the keys themselves do not have to be known, they can be literals or bindings, whichever.

> Btw yes, I don't want to contradict on a more proper example, I'm just trying to learn a bit more from your answers :smile:

All good.  :-)

> Said that, what do you think for a more proper example to put into the docs ?

If you could get this to work, I'd be so happy:

```elixir
query = from q in unsafe_fragment("public.blah")
query = join(query, :inner, [q], s in unsafe_fragment("someschema.sometable"), s.id == q.id)
```

Or this:

```elixir
query = from q in Blah, group_by: q.name, select: max(q.value)
query = unsafe_query("? UNION SELECT 1", subquery(query))
```
xadhoom: Well having the "count" known a compile time when you write a sorting function for all your models, even with branching is not really feasible, unless you put a max on how many fields you may sort at a time (which can be reasonable after all).

I'll try to try your examples and report back :)
OvermindDL1: Also an idea, still disallow string interpolation in unsafe_fragment and go back to `?` inputs, but instead combine them *without* escaping them (which is what `fragment/?` currently does).  The API would be a lot more consistent then and if someone wants the original functionality as it stands then they could just do `unsafe_fragment("?", "blah blah blah")`.  but for those that like to use it more like functions then it would be a lot nicer.

_/me is still unsure if wants it added, need to see how well it handles the truly 'currently impossible' cases..._
xadhoom: Well, in your example also the plain fragment/1 with interpolation (yes does not makes sense, but just to check) blames about "cannot use outside match clause" and so on.

So mine does, since it is just a fragment/1 underneath.

Before going on a different path, I would like some feedback also from top mantainers in order to have a more shared/general view on where to go.

After all I just did what I understood from @josevalim from a quick email exchange on https://groups.google.com/forum/#!topic/elixir-ecto/8aLKRvp5P3k
josevalim: @xadhoom please check builder/join and builder/select, they explicitly handle fragments in some places, so we will need to do that there too and add some tests. Good news is that the implementation should be equally straight-forward.

Patch:
diff --git a/lib/ecto/embedded.ex b/lib/ecto/embedded.ex
index ff2de52b..13601c0f 100644
--- a/lib/ecto/embedded.ex
+++ b/lib/ecto/embedded.ex
@@ -48,20 +48,18 @@ defmodule Ecto.Embedded do
   loaded into the schema struct afterwards.
   """
   def prepare(changeset, adapter, repo_action) do
-    %{changes: changes, data: %{__struct__: schema}} = changeset
-    prepare(changeset, Map.take(changes, schema.__schema__(:embeds)), adapter, repo_action)
+    %{changes: changes, data: %{__struct__: schema}, types: types} = changeset
+    prepare(Map.take(changes, schema.__schema__(:embeds)), types, adapter, repo_action)
   end
 
-  defp prepare(changeset, embeds, _adapter, _repo_action) when embeds == %{} do
-    changeset
+  defp prepare(embeds, _types, _adapter, _repo_action) when embeds == %{} do
+    embeds
   end
 
-  defp prepare(%{types: types} = changeset, embeds, adapter, repo_action) do
-    update_in changeset.changes, fn changes ->
-      Enum.reduce embeds, changes, fn {name, changeset}, acc ->
-        {:embed, embed} = Map.get(types, name)
-        Map.put(acc, name, prepare_each(embed, changeset, adapter, repo_action))
-      end
+  defp prepare(embeds, types, adapter, repo_action) do
+    Enum.reduce embeds, embeds, fn {name, changeset}, acc ->
+      {:embed, embed} = Map.get(types, name)
+      Map.put(acc, name, prepare_each(embed, changeset, adapter, repo_action))
     end
   end
 
@@ -103,9 +101,11 @@ defmodule Ecto.Embedded do
   end
 
   defp to_struct(%Changeset{} = changeset, action, %{related: schema}, adapter) do
-    %{data: struct, changes: changes} = prepare(changeset, adapter, action)
+    %{data: struct, changes: changes} = changeset
+    embeds = prepare(changeset, adapter, action)
 
     changes
+    |> Map.merge(embeds)
     |> autogenerate_id(struct, action, schema, adapter)
     |> autogenerate(action, schema)
     |> apply_embeds(struct)
diff --git a/lib/ecto/query/api.ex b/lib/ecto/query/api.ex
index 7703466e..fe981236 100644
--- a/lib/ecto/query/api.ex
+++ b/lib/ecto/query/api.ex
@@ -271,6 +271,49 @@ defmodule Ecto.Query.API do
   """
   def fragment(fragments), do: doc! [fragments]
 
+  @doc ~S"""
+  Send an unsafe fragment directly to the database.
+
+  WARNING: sending an unsafe fragment may expose your application to sql
+  injection attacks. Think twice before using it and try to reach the same
+  result using the safe fragment/1 API.
+
+  Sometimes it is not possible to represent all possible databases queries
+  using Ecto's syntax or fragment/1, expecially when dealing with specific
+  database construct and dynamic fields.
+
+  To overcome this, it is possible to build the expression as a binary
+  outside the query expression and send it to the database by interpolating
+  the value in the query.
+
+      def window_sorting(sorters) do
+        sort = sorters |> Enum.map(fn %{sort: key, direction: dir} ->
+          dir = case dir do
+            :asc -> "ASC"
+            :desc -> "DESC"
+          end
+          "#{key} #{dir}"
+        end
+        |> Enum.join(",")
+
+        expr = "ROW_NUMBER() OVER(ORDER BY #{sort})"
+        from p in Post,
+          select: %{p | id: unsafe_fragment(^expr)}
+      end
+
+  In the example above, we are using the row_number() sql window function to
+  do a particular type of ordering, using a list of sorters which are not
+  known at compile time.
+
+  It is very important to keep in mind that no input sanitization is done
+  by Ecto, so the application may be open to sql injection attacks if the
+  expression is not built properly.
+
+  Only an interpolated string (binary) is supported, any other value will
+  result into a compilation error or ArgumentError during runtime.
+  """
+  def unsafe_fragment(fragment), do: doc! [fragment]
+
   @doc """
   Allows a field to be dynamically accessed.
 
diff --git a/lib/ecto/query/builder.ex b/lib/ecto/query/builder.ex
index b970c54a..af55f2ab 100644
--- a/lib/ecto/query/builder.ex
+++ b/lib/ecto/query/builder.ex
@@ -82,6 +82,23 @@ defmodule Ecto.Query.Builder do
     {{:{}, [], [:fragment, [], merge_fragments(pieces, frags)]}, params_acc}
   end
 
+  def escape({:unsafe_fragment, _, [{:^, _, [var]} = _expr]}, _type, params_acc, _vars, _env) do
+    expr = quote do
+      Ecto.Query.Builder.unsafe_fragment!(unquote(var))
+    end
+    {{:{}, [], [:fragment, [], [{:raw, expr}]]}, params_acc}
+  end
+
+  def escape({:unsafe_fragment, _, [_ | _]}, _type, _params_acc, _vars, _env) do
+    error! """
+    unsafe_fragment(...) expects a single argument to be interpolated as ^argument
+    
+    The argument is a string that will be sent to the database as is. For this reason,
+    use this feature with extreme care and instead prefer to use fragment as much 
+    as possible.
+    """
+  end
+
   # interval
 
   def escape({:from_now, meta, [count, interval]}, type, params_acc, vars, env) do
@@ -536,13 +553,46 @@ defmodule Ecto.Query.Builder do
   """
   def keyword!(kw) do
     unless Keyword.keyword?(kw) do
-      raise ArgumentError, "to prevent sql injection, only a keyword list may be interpolated " <>
-                           "as the first argument to `fragment/1` with the `^` operator, got `#{inspect kw}`"
+      message = """
+      to prevent sql injection, only a keyword list may be interpolated
+      as the first argument to `fragment/1` with the `^` operator, got `#{inspect kw}`
+      """
+      message = if is_binary(kw) do
+        message <> """
+
+        For interpolated strings use `unsafe_fragment/1` to pass the argument
+        as-is to the database, paying attention to possible sql injection attack
+        vectors as no escaping is done by Ecto.
+        
+        Use `unsafe_fragment/1` only as last resort and wisely.
+        """
+      else
+        message
+      end
+      raise ArgumentError, message: message
     end
 
     kw
   end
 
+  @doc """
+  Called by escaper at runtime to verify binaries.
+  """
+  def unsafe_fragment!(v) do
+    unless is_binary(v) do
+      raise ArgumentError, """
+        `unsafe_fragment/1` expects only an interpolated string.
+          You should build your binary and pass as an interpolated value with the `^` operator.
+
+          WARNING: interpolating the fragment may open you to sql injection attacks!
+
+          You may want to use `fragment/1`, which also prevents sql injection.
+        """
+    end
+
+    v
+  end
+
   @doc """
   Called by escaper at runtime to verify that value is an atom.
   """
diff --git a/lib/ecto/query/builder/join.ex b/lib/ecto/query/builder/join.ex
index c068a8bc..95b4ecb8 100644
--- a/lib/ecto/query/builder/join.ex
+++ b/lib/ecto/query/builder/join.ex
@@ -56,6 +56,11 @@ defmodule Ecto.Query.Builder.Join do
     {:_, expr, nil, params}
   end
 
+  def escape({:unsafe_fragment, _, [_]} = expr, vars, env) do
+    {expr, {params, :acc}} = Builder.escape(expr, :any, {%{}, :acc}, vars, env)
+    {:_, expr, nil, params}
+  end
+
   def escape({:__aliases__, _, _} = module, _vars, _env) do
     {:_, {nil, module}, nil, %{}}
   end
diff --git a/lib/ecto/query/builder/select.ex b/lib/ecto/query/builder/select.ex
index a119ec09..582a61b1 100644
--- a/lib/ecto/query/builder/select.ex
+++ b/lib/ecto/query/builder/select.ex
@@ -100,6 +100,10 @@ defmodule Ecto.Query.Builder.Select do
     escape_with_type(expr, type, params_take, vars, env)
   end
 
+  defp escape({:type, _, [{:unsafe_fragment, _, [_]} = expr, type]}, params_take, vars, env) do
+    escape_with_type(expr, type, params_take, vars, env)
+  end
+
   defp escape({:type, _, [{agg, _, [_ | _]} = expr, type]}, params_take, vars, env)
        when agg in ~w(avg count max min sum)a do
     escape_with_type(expr, type, params_take, vars, env)
diff --git a/lib/ecto/repo/schema.ex b/lib/ecto/repo/schema.ex
index ab3a69c4..9ec5ebc9 100644
--- a/lib/ecto/repo/schema.ex
+++ b/lib/ecto/repo/schema.ex
@@ -195,13 +195,14 @@ defmodule Ecto.Repo.Schema do
       changeset = process_parents(changeset, parents, opts)
 
       if changeset.valid? do
-        changeset = Ecto.Embedded.prepare(changeset, adapter, :insert)
+        embeds = Ecto.Embedded.prepare(changeset, adapter, :insert)
 
         autogen_id = schema.__schema__(:autogenerate_id)
         metadata = metadata(struct, autogen_id, opts)
 
+        changes = Map.merge(changeset.changes, embeds)
         {changes, extra, return_types, return_sources} =
-          autogenerate_id(autogen_id, changeset.changes, return_types, return_sources, adapter)
+          autogenerate_id(autogen_id, changes, return_types, return_sources, adapter)
         {changes, autogen} =
           dump_changes!(:insert, Map.take(changes, fields), schema, extra, dumper, adapter)
         on_conflict =
@@ -211,8 +212,9 @@ defmodule Ecto.Repo.Schema do
         args = [repo, metadata, changes, on_conflict, return_sources, opts]
         case apply(changeset, adapter, :insert, args) do
           {:ok, values} ->
+            values = extra ++ values
             changeset
-            |> load_changes(:loaded, return_types, extra ++ values, autogen, adapter, metadata)
+            |> load_changes(:loaded, return_types, values, embeds, autogen, adapter, metadata)
             |> process_children(children, user_changeset, opts)
           {:error, _} = error ->
             error
@@ -272,9 +274,9 @@ defmodule Ecto.Repo.Schema do
         changeset = process_parents(changeset, parents, opts)
 
         if changeset.valid? do
-          changeset = Ecto.Embedded.prepare(changeset, adapter, :update)
+          embeds = Ecto.Embedded.prepare(changeset, adapter, :update)
 
-          original = Map.take(changeset.changes, fields)
+          original = changeset.changes |> Map.merge(embeds) |> Map.take(fields)
           {changes, autogen} = dump_changes!(:update, original, schema, [], dumper, adapter)
 
           metadata = metadata(struct, schema.__schema__(:autogenerate_id), opts)
@@ -290,7 +292,7 @@ defmodule Ecto.Repo.Schema do
           case apply(changeset, adapter, action, args) do
             {:ok, values} ->
               changeset
-              |> load_changes(:loaded, return_types, values, autogen, adapter, metadata)
+              |> load_changes(:loaded, return_types, values, embeds, autogen, adapter, metadata)
               |> process_children(children, user_changeset, opts)
             {:error, _} = error ->
               error
@@ -376,7 +378,8 @@ defmodule Ecto.Repo.Schema do
       args = [repo, metadata, filters, opts]
       case apply(changeset, adapter, :delete, args) do
         {:ok, values} ->
-          {:ok, load_changes(changeset, :deleted, [], values, [], adapter, metadata).data}
+          changeset = load_changes(changeset, :deleted, [], values, %{}, [], adapter, metadata)
+          {:ok, changeset.data}
         {:error, _} = error ->
           error
         {:invalid, constraints} ->
@@ -619,23 +622,25 @@ defmodule Ecto.Repo.Schema do
     end
   end
 
-  defp load_changes(%{changes: changes} = changeset,
-                    state, types, values, autogen, adapter, %{source: source}) do
+  defp load_changes(changeset, state, types, values, embeds, autogen, adapter, metadata) do
+    %{changes: changes} = changeset
+    %{source: source} = metadata
+
     # It is ok to use types from changeset because we have
     # already filtered the results to be only about fields.
     data =
       changeset.data
-      |> merge_changes(changes)
+      |> Map.merge(changes)
+      |> Map.merge(embeds)
       |> merge_autogen(autogen)
       |> apply_metadata(state, source)
       |> load_each(values, types, adapter)
     Map.put(changeset, :data, data)
   end
 
-  defp merge_changes(data, changes),
-    do: Map.merge(data, changes)
-  defp merge_autogen(data, autogen),
-    do: Enum.reduce(autogen, data, fn {k, v}, acc -> %{acc | k => v} end)
+  defp merge_autogen(data, autogen) do
+    Enum.reduce(autogen, data, fn {k, v}, acc -> %{acc | k => v} end)
+  end
 
   defp apply_metadata(%{__meta__: meta} = data, state, source) do
     %{data | __meta__: %{meta | state: state, source: source}}


Test patch:
diff --git a/test/ecto/query/builder/join_test.exs b/test/ecto/query/builder/join_test.exs
index 869036a5..d79a8262 100644
--- a/test/ecto/query/builder/join_test.exs
+++ b/test/ecto/query/builder/join_test.exs
@@ -72,6 +72,43 @@ defmodule Ecto.Query.Builder.JoinTest do
     join("posts", :left, [p], c in subquery(subquery, prefix: "sample"), true)
   end
 
+  test "accepts interpolated binary as unsafe fragment" do
+    frag = "comments"
+    join("posts", :left, [p], c in unsafe_fragment(^frag), true)
+  end
+
+  test "raises on non interpolated argument" do
+    assert_raise Ecto.Query.CompileError, ~r/expects a single argument/, fn ->
+      escape(quote do
+        join("posts", :left, [p], c in unsafe_fragment("comments"), true)
+      end, [], __ENV__)
+    end
+
+    assert_raise Ecto.Query.CompileError, ~r/expects a single argument/, fn ->
+      escape(quote do
+        join("posts", :left, [p], c in unsafe_fragment(["post"]), true)
+      end, [], __ENV__)
+    end
+
+    assert_raise Ecto.Query.CompileError, ~r/expects a single argument/, fn ->
+      escape(quote do
+        join("posts", :left, [p], c in unsafe_fragment(["$eq": "foo"]), true)
+      end, [], __ENV__)
+    end
+  end
+
+  test "raises on invalid interpolated unsafe fragments" do
+    frag = ["comments", "authors"]
+    assert_raise ArgumentError, ~r/expects only an interpolated string/, fn ->
+      join("posts", :left, [p], c in unsafe_fragment(^frag), true)
+    end
+
+    frag = ["comments": "authors"]
+    assert_raise ArgumentError, ~r/expects only an interpolated string/, fn ->
+      join("posts", :left, [p], c in unsafe_fragment(^frag), true)
+    end
+  end
+
   test "raises on invalid qualifier" do
     assert_raise ArgumentError,
                  ~r/invalid join qualifier `:whatever`/, fn ->
diff --git a/test/ecto/query/builder/select_test.exs b/test/ecto/query/builder/select_test.exs
index 993dfdae..13df72fd 100644
--- a/test/ecto/query/builder/select_test.exs
+++ b/test/ecto/query/builder/select_test.exs
@@ -159,4 +159,44 @@ defmodule Ecto.Query.Builder.SelectTest do
       assert Macro.to_string(query.select.expr) =~ "merge"
     end
   end
+
+  describe "select with type fragments" do
+    test "supports typed fragment" do
+      from p in "posts", select: type(fragment("cost"), :decimal)
+    end
+
+    test "supports typed fragment with interpolated keyword" do
+      foo = ["$eq": 42]
+      from p in "posts", select: type(fragment(^foo), :decimal)
+    end
+
+    test "raises at runtime on invalid interpolation" do
+      foo = "cost"
+      assert_raise ArgumentError, ~r/only a keyword list may be interpolated/, fn ->
+        from p in "posts", select: type(fragment(^foo), :decimal)
+      end
+    end
+  end
+
+  describe "select with type unsafe fragments" do
+    test "raises on not interpolated binary" do
+      assert_raise Ecto.Query.CompileError, ~r/expects a single argument/, fn ->
+        escape(quote do
+          select([c], type(unsafe_fragment("cost"), :decimal))
+        end, [], __ENV__)
+      end
+    end
+
+    test "raises on interpolated keyword" do
+      assert_raise ArgumentError, ~r/ expects only an interpolated string/, fn ->
+        foo = ["$eq": 42]
+        from p in "posts", select: type(unsafe_fragment(^foo), :decimal)
+      end
+    end
+
+    test "accepts an interpolated binary" do
+      foo = "cost"
+      from p in "posts", select: type(unsafe_fragment(^foo), :decimal)
+    end
+  end
 end
diff --git a/test/ecto/query_test.exs b/test/ecto/query_test.exs
index f4f2913d..60151b95 100644
--- a/test/ecto/query_test.exs
+++ b/test/ecto/query_test.exs
@@ -364,7 +364,14 @@ defmodule Ecto.QueryTest do
 
   describe "fragment/1" do
     test "raises at runtime when interpolation is not a keyword list" do
-      assert_raise ArgumentError, ~r/only a keyword list.*1 = \?/, fn ->
+      assert_raise ArgumentError, ~r/only a keyword list.*1 = \?/s, fn ->
+        clause = ["1 = ?"]
+        from p in "posts", where: fragment(^clause)
+      end
+    end
+
+    test "raises at runtime when interpolation is a binary string" do
+      assert_raise ArgumentError, ~r/unsafe_fragment\/1/, fn ->
         clause = "1 = ?"
         from p in "posts", where: fragment(^clause)
       end
@@ -375,4 +382,87 @@ defmodule Ecto.QueryTest do
              ~s[#Ecto.Query<from p in \"posts\", where: fragment("héllò")>]
     end
   end
+
+  describe "unsafe_fragment/1" do
+    test "raises with non interpolated binary fragment" do
+      message = ~r"expects a single argument to be interpolated"
+      assert_raise Ecto.Query.CompileError, message, fn ->
+        quote_and_eval(
+          from p in "posts", where: unsafe_fragment("1 = 1")
+        )
+      end
+    end
+
+    test "raises with non interpolated binary and params" do
+      message = ~r"expects a single argument to be interpolated"
+      assert_raise Ecto.Query.CompileError, message, fn ->
+        quote_and_eval(
+          from p in "posts", where: unsafe_fragment("1 = ?", 1)
+        )
+      end
+    end
+
+    test "raises with non interpolated, non empty list" do
+      message = ~r"expects a single argument to be interpolated"
+      assert_raise Ecto.Query.CompileError, message, fn ->
+        quote_and_eval(
+          from p in "posts", where: unsafe_fragment(["foo", "bar"])
+        )
+      end
+    end
+
+    test "raises with non interpolated, empty list" do
+      message = ~r"expects a single argument to be interpolated"
+      assert_raise Ecto.Query.CompileError, message, fn ->
+        quote_and_eval(
+          from p in "posts", where: unsafe_fragment([])
+        )
+      end
+    end
+
+    test "raises with non interpolated keyword list" do
+      message = ~r"expects a single argument to be interpolated"
+      assert_raise Ecto.Query.CompileError, message, fn ->
+        quote_and_eval(
+          from p in "posts", where: unsafe_fragment(["foo": "bar"])
+        )
+      end
+    end
+
+    test "raises at runtime when interpolation is a keyword list" do
+      assert_raise ArgumentError, ~r"expects only an interpolated string", fn ->
+        clause = ["foo": "bar"]
+        from p in "posts", where: unsafe_fragment(^clause)
+      end
+    end
+
+    test "raises at runtime when interpolation is a non empty list" do
+      assert_raise ArgumentError, ~r"expects only an interpolated string", fn ->
+        clause = ["foo", "bar"]
+        from p in "posts", where: unsafe_fragment(^clause)
+      end
+    end
+
+    test "raises at runtime when interpolation is an empty list" do
+      assert_raise ArgumentError, ~r"expects only an interpolated string", fn ->
+        clause = []
+        from p in "posts", where: unsafe_fragment(^clause)
+      end
+    end
+
+    test "works with interpolated binary fragment" do
+      val = 1
+      clause = "1 = #{val}"
+      query = from p in "posts", where: unsafe_fragment(^clause)
+
+      assert inspect(query) ==
+            ~s[#Ecto.Query<from p in \"posts\", where: fragment(\"1 = 1\")>]
+    end
+
+    test "keeps UTF-8 encoding" do
+      clause = "héllò"
+      assert inspect(from p in "posts", where: unsafe_fragment(^clause)) ==
+             ~s[#Ecto.Query<from p in \"posts\", where: fragment("héllò")>]
+    end
+  end
 end
diff --git a/test/ecto/repo/embedded_test.exs b/test/ecto/repo/embedded_test.exs
index afbcc751..15952c5c 100644
--- a/test/ecto/repo/embedded_test.exs
+++ b/test/ecto/repo/embedded_test.exs
@@ -22,12 +22,22 @@ defmodule Ecto.Repo.EmbeddedTest do
     end
   end
 
+  defmodule MyAssoc do
+    use Ecto.Schema
+
+    schema "my_assocs" do
+      field :x
+      field :my_assoc_id
+    end
+  end
+
   defmodule MySchema do
     use Ecto.Schema
 
     schema "my_schema" do
       embeds_one :embed, MyEmbed, on_replace: :delete
       embeds_many :embeds, MyEmbed, on_replace: :delete
+      has_one :assoc, MyAssoc
     end
   end
 
@@ -96,6 +106,22 @@ defmodule Ecto.Repo.EmbeddedTest do
     refute changeset.valid?
   end
 
+  test "returns untouched changeset on invalid child association" do
+    invalid_assoc =
+      put_in(%MyAssoc{}.__meta__.context, {:invalid, [unique: "my_assocs_foo_index"]})
+      |> Ecto.Changeset.change()
+      |> Ecto.Changeset.unique_constraint(:foo)
+
+    changeset =
+      %MySchema{}
+      |> Ecto.Changeset.change
+      |> Ecto.Changeset.put_assoc(:assoc, invalid_assoc)
+      |> Ecto.Changeset.put_embed(:embed, %MyEmbed{x: "xyz"})
+
+    {:error, changeset} = TestRepo.insert(changeset)
+    assert %Ecto.Changeset{} = changeset.changes.embed
+  end
+
   test "handles nested embeds on insert" do
     embed =
       %MyEmbed{x: "xyz"}