Repository: elixir-ecto/ecto
Pull request #4410 (2024-05-01 11:44:50)

Add :defaults_to_struct option to embeds_one
This is related to #2634, but I think I found a good reason to suggest a change like this.

A quite common pattern I've seen to store "user settings" is to have a `settings` embed in the user schema, like this:

```elixir
defmodule User do
  use Ecto.Schema

  schema "users" do
    embeds_one :settings, Settings do
      field :dark_mode, :boolean, default: false
    end
  end
end
```

So then in one place of the code you call `if user.settings.dark_mode, do: ...` but then you realize this breaks if you never explicitly set `:settings` embed, since then `user.settings` become nil.

That, however, is not an issue currently because you can easily make sure to set that field when creating the user or adding a default `{}` value on your postgres database and any new field you add will work as long as you set a default.

The problem happens, however, if you want to nest embeds. Let's say you want to now add some notification settings but you want to organize it with a nested embedded schema, like so:

```elixir
defmodule User do
  use Ecto.Schema

  schema "users" do
    embeds_one :settings, Settings do
      field :dark_mode, :boolean, default: false
      
      embeds_one :notifications, Notifications do
        field :email, :boolean, default: true
        field :sms, :boolean, default: false
      end
    end
  end
end
```

Now you have a problem. While if this was a greenfield project, setting up at creation or at the database level might be fine, for an existing project this will require a backfill. The problem is that any new nested embed will require a backfill, which is much worse than what you get with plain `field` where you can add a default. So your option is probably to just flatten the settings object.

While I see the argument for a flatter structure I also think that nesting can be useful in terms of organization. So to help with that and save people some unecessary nil checks, I came with the idea of adding an option (so not to break backwards compatibility) to embeds_one where it will interpret the lack of a field as an empty map value when loading an embed, so the above problem could be represented as follows:

```elixir
defmodule User do
  use Ecto.Schema

  schema "users" do
    embeds_one :settings, Settings do
      field :dark_mode, :boolean, default: false
      
      embeds_one :notifications, Notifications, null: false do
        field :email, :boolean, default: true
        field :sms, :boolean, default: false
      end
    end
  end
end
```

Hope this makes sense and you like the approach here 😄 

josevalim: @bamorim, thank you! If we are going down this path, we should probably build it only once and have it cached in the association struct, instead of building it every time.

I'd also like to discuss the name `null: false`. We don't use it anywhere in the schema DSL (as it is a database specific term). Perhaps there are other options?
bamorim: > @bamorim, thank you! If we are going down this path, we should probably build it only once and have it cached in the association struct, instead of building it every time.

Ah, so we build and treat it like the field's default option behaviour?

> I'd also like to discuss the name `null: false`. We don't use it anywhere in the schema DSL (as it is a database specific term). Perhaps there are other options?

I didn't put any thought to this name only that I wanted to "avoid nulls". Maybe we can go with something related to `default` like `build_default`. I thought about `default: true`, but then it would be different from how `default` works elsewhere. Maybe `with_default: true` could also be an option?

bamorim: @josevalim oh, with your feedback it seems like things became much cleaner actually. See if that is what you were looking for. :)
josevalim: I am not sure if this version will work as you expect because you can't create an empty embed to pass a default without externally defining the embedding module first :(
bamorim: > I am not sure if this version will work as you expect because you can't create an empty embed to pass a default without externally defining the embedding module first :(

Which case this would fail? Inline embeds are defined before calling __embeds_one__, so it should work, no?
josevalim: But you want to pass a struct as a default to the embed that defines it, no? Or does Ecto guarantee the defmodule happens before hand? If so, that’s a hit weird. Can you post an example of how you would use it, to make sure we are in the same page?
bamorim: Ah, I think I see the miscommunication here. So, my line of thought is not to provide a "configurable" default, but rather just generate the embed with it's defaults. This is why in my current example I went with "with_default" option instead of "default" because this don't allow people to manually specify what they have.

So, all this does is pretty much defaults to whatever `%MyEmbeddedSchema{}` would default to instead of `nil`.

So with this:

```elixir
defmodule User do
  use Ecto.Schema
  
  schema "users" do
    embeds_one :settings, Settings, with_default: true
  end
end

defmodule Settings do
  use Ecto.Schema do
  
  embedded_schema do
    field :dark_mode, :boolean, default: false
    field :accent_color, :string
    
    embeds_one :notifications, Notifications do
      field :email, :boolean, default: true
      field :sms, :boolean, default: false
    end
  end
end
```

We would have the following

```elixir
assert %User{
  settings: %Settings{
    dark_mode: false,
    accent_color: nil,
    notifications: %Settings.Notifications{
      email: true,
      sms: false
    }
  }
} = %User{} # Or Repo.get(User, id) when settings is `NULL` in the database
```
josevalim: You are right, I got confused. :) Thank you for clarifying.

We just need to decide the option name, I am not happy with `with_default: ...` 
bamorim: @josevalim so, what you think about the idea of using `:default` (`default: true`) instead of `:with_default` and with even the possibility of specifying custom value for the `embeds_one/3` case, like I described above?

We can assert on the value on the `:default` option to be either `true` or `%Schema{...}` and explain accordingly.

What I like about that is that in the end this is really similar to setting a default on a field with just one exception/extension to make it work with inline embeds, so I think it will be natural for people when thinking about it.
josevalim: To me it is a bit weird to hijack `default: true` as a special meaning. I'd like to hear suggestions from @v0idpwn and @greg-rychlewski in relation to naming. Thank you @bamorim.
bamorim: We can also have `default: %Schema{...}` for the `embeds_one/3` (non-inline) case and `with_default: true` as a "shorthand" that works for both cases. That way there's no special value on `default:` but rather two different options you can choose from.

But I'll wait on the suggestions from others.
greg-rychlewski: Something like `set_default: true/false` might work. I agree `default: true` looks a bit odd to me because of how default works for "regular" fields.
v0idpwn: I think it could be a nice idea to do a small hack on the `embeds_one` macro to make it accept the struct before it was defined. It's dirtier inside but IMO would be the best API.
greg-rychlewski: It seems a bit confusing to me personally. We are essentially telling them they can set the default one way or not set it at all. That behaviour is more in line with a boolean.
bamorim: > It seems a bit confusing to me personally. We are essentially telling them they can set the default one way or not set it at all. That behaviour is more in line with a boolean.

But for non in-line embeds it is useful to set different defaults as the example I gave above.

Unfortunately it is just too hard to set defaults for in-line embeds.

Alternatives I can think are:
1. This current implementation (only `:default` with the inline caveat) 
2. Don't allow defaults for in-line embeds at all
3. Introduce the second option `:with_default` as boolean and keep the `:default` as value only for non inline embeds
4. Drop the support for custom defaults for both and keep only the `:with_default`

Which you prefer?
greg-rychlewski: My comment was only about the inline embed proposal where the option is called `:default` and everything raises except for `%Embed{}` or `nil`. To me that is better served by a boolean.

I am not sure what the best solution to this issue is. I am currently thinking 2 different options is best for the following reasons:

- module-based embeds and inline embeds should both be allowed to have defaults
- module-based embeds should be allowed to have different defaults because they can be embedded inside different schemas
- I can't think of why it would make sense to allow a default for inline that is different than the default struct. If anything it seems confusing to me
- I don't think it would make a tonne of sense to name the module-based option something other than `:default`. The behaviour is exactly that of the other default options
- I don't think it would make sense to call the inline option `:default` since it can only take 1 value other than `nil`
greg-rychlewski: Now I'm also thinking it's kind of confusing to have a `:default` for the module-based embeds when the struct has its own defaults.

Like if you define `default: %Embed{field: "default"}` and then the embed module defines a default for `:other_field` as "other default". The actual default value is %Embed{field: "default", other_field: "other default"}` but it might not be immediately obvious. You always have to cross reference the two modules. Unless you are saying to overwrite all the embed's own defaults. Then that is confusing for different reasons.
greg-rychlewski: This will probably not be a popular opinion, but I am thinking something like `set_default_struct: true/false` for both module-based embeds and inline embeds would be best. And it just sets the default struct for the embed. The other stuff has a lot to think about in terms of interactions and seems a bit confusing to me.

But I don't think it's so bad to try to block it. If everyone else sees a lot of value in the current direction that's ok with me.
josevalim: Agreed a boolean flag is better, my concern is only about the naming. My suggestions (after a night of sleep) are: `defaults_to_itself: true`, `defaults_to_struct: true`, or `defaults_to_nil: false`.
bamorim: I like `defaults_to_nil: false` more and is closer to my initial proposal.

But to be sure, we are thinking about dropping the option of specifying a custom value even for non-inline embeds?

Im fine with that but I do think that it would have its use case. The argument of %Embed{this_field: "something} actually having a second field set because of a previously defined struct would be true everywhere.

In fact, for the non inline case you are just passing a value that could come from anywhere, being a module attribute or a function call (although it would be called in compile time) so I'm not 100% sure it is that confusing. There is no special meaning syntax there.

With all that said, just the boolean option is already a great improvement and solves the main issue of adding extra fields to existing embeds in an easy way without requiring data migrations. 
v0idpwn: There two places where we need consistency:

1) **Option names/values consistency:** the `:default` shouldn't have special cases
2) **Same API consistency:** inline embeds_one shouldn't behave differently or have different options than non-inline

I think these preconditions give us the following alternatives:
a) a new option, receiving a boolean, like `:defaults_to_nil` or `defaults_to_struct`
b) `:default`, but then we need to support receiving a struct, and this has different problems as outlined by @greg-rychlewski 

I think middle of the path solutions, like supporting struct for a case and boolean for the other, make things too wonky. I'm okay with losing a bit of customizability for non-inline, and then vote for `a)`.
bamorim: Ok, just reverted some commits and made sure that defaults behaves like other fields (if the underlying JSON actually explicitly sets the field to nil the load will respect that). This means the main goal of being able to add a new nested embed without data migrations is still possible but we also allow people to manually set something to nil if they want.

However, if a normal schema with a backing postgres table has already the jsonb field and it is nullable, this won't override and data migrations will be necessary. However, when adding new entries will have the default which is good.

This also reduced the complexity of the solution a lot by not requiring to save the default in the Embedded struct to then replace when loading.

Last but not least, I went with `defaults_to_struct` just because the code I had already was using the "key: true" logic, but if we want I can revert to checking for `defaults_to_nil` if that's the preference. But I have a wedding to attend, I can do that change on Sunday or tomorrow morning UTC+1.
greg-rychlewski: we should add the option to the docs as well
bamorim: @greg-rychlewski I added some documentation about the option.

Patch:
diff --git a/lib/ecto/schema.ex b/lib/ecto/schema.ex
index 151bbb3d..4d593369 100644
--- a/lib/ecto/schema.ex
+++ b/lib/ecto/schema.ex
@@ -1648,6 +1648,12 @@ defmodule Ecto.Schema do
       selecting the whole struct in a query, such as `from p in Post, select: p`.
       Defaults to `true`.
 
+    * `:defaults_to_struct` - When true, the field will default to the initialized
+      struct instead of nil, the same you would get from something like `%Order.Item{}`.
+      One important thing is that if the underlying data is explicitly nil when loading
+      the schema, it will still be loaded as nil, similar to how `:default` works in fields.
+      Defaults to `false`.
+
   ## Examples
 
       defmodule Order do
@@ -2223,11 +2229,19 @@ defmodule Ecto.Schema do
     Module.put_attribute(mod, :ecto_changeset_fields, {name, {:assoc, struct}})
   end
 
-  @valid_embeds_one_options [:on_replace, :source, :load_in_query]
+  @valid_embeds_one_options [:on_replace, :source, :load_in_query, :defaults_to_struct]
 
   @doc false
   def __embeds_one__(mod, name, schema, opts) when is_atom(schema) do
     check_options!(opts, @valid_embeds_one_options, "embeds_one/3")
+
+    opts =
+      if Keyword.get(opts, :defaults_to_struct) do
+        Keyword.put(opts, :default, schema.__schema__(:loaded))
+      else
+        opts
+      end
+
     embed(mod, :one, name, schema, opts)
   end
 


Test patch:
diff --git a/test/ecto/embedded_test.exs b/test/ecto/embedded_test.exs
index 000164aa..162107b6 100644
--- a/test/ecto/embedded_test.exs
+++ b/test/ecto/embedded_test.exs
@@ -34,6 +34,15 @@ defmodule Ecto.EmbeddedTest do
     end
   end
 
+  defmodule Settings do
+    use Ecto.Schema
+
+    embedded_schema do
+      field :dark_mode, :boolean, default: false
+      embeds_one :default_post, Post, defaults_to_struct: true
+    end
+  end
+
   test "__schema__" do
     assert Author.__schema__(:embeds) ==
       [:profile, :post, :posts]
@@ -63,6 +72,12 @@ defmodule Ecto.EmbeddedTest do
     assert %UUIDSchema{uuid: ^uuid, authors: [%Author{}]} =
              Ecto.embedded_load(UUIDSchema, %{"uuid" => uuid, "authors" => [%{}]}, :json)
 
+    assert %Settings{dark_mode: false, default_post: %Post{}} =
+             Ecto.embedded_load(Settings, %{}, :json)
+
+    assert %Settings{dark_mode: false, default_post: nil} =
+             Ecto.embedded_load(Settings, %{"default_post" => nil}, :json)
+
     assert_raise ArgumentError,
                  ~s[cannot load `"ABC"` as type Ecto.UUID for field `uuid` in schema Ecto.EmbeddedTest.UUIDSchema],
                  fn ->