Move preprocessing out of adapters is causing problems for the MongoDB Adapter

[scottmessinger]

Hi! I'm working on upgrading the Mongo adapter to be compatible with Ecto 3 and #2312 is causing a problem I'm not sure how to fix and I was wondering if you all had any ideas.

Prior to this change, if a field didn't exist in the document retrieved from the database, Ecto would use the default value. e.g. If the database has this:

%{
  _id: 1,
  title: "Blog Post",
}

and the schema is:

defmodule App.Post do 
  schema "posts" do
    field(:title, :string),
    field(:year, :number, default: 2020)
  end
end

And you were to fetch the doc:

doc = App.Repo.get(App.Post, 1)

In Ecto 2.2, Ecto fills in the non-existent field with the default. E.g

doc == %{
  _id: 1,
  title: "Blog Post",
  year: 2020,
}

After this change, Ecto does not fill in the field with the default. In this example, the year is nil. E.g.

doc == %{
  _id: 1,
  title: "Blog Post",
  year: nil,
}

In our app, we're relying on the default throughout our app and getting a ton of errors as fields we assume to be maps or lists are now nil.

Any idea how we can restore the old functionality with this new model?

[josevalim]

Can you please tell me what adapter.execute is returning in your case?

[scottmessinger]

Sure! It's returning {count, rows}. For the example above, it would return the following:

{ 1, [
  [
    1,
    "Blog Post",
    nil
  ]
]}

In 2.2, it would return a list of structs instead of a list of lists. Is that the information you're looking for?

[josevalim]

I think we will have to change ecto to allow maps besides lists internally when preprocessing - if we don't allow them yet. PRs are welcome! ❤️

[scottmessinger]

@josevalim Okay! Would there be an option to allow an atom to be passed instead of nil that would allow the processing step to convert to the field's default? E.g. instead of

{ 1, [
  [
    1,
    "Blog Post",
    nil
  ]
]}

I would pass

{ 1, [
  [
    1,
    "Blog Post",
    :__unknown__
  ]
]}

and then we'd just need to change the processor functions to fill in :__unknown__ with the default?

[josevalim]

I think a map is more appropriate here, so I would try to go down this route instead. :)

[scottmessinger]

@josevalim My concern here is changing the process code to accept a map is going to be a much bigger change and involved far, far more lines of code than handling a different type of value. I'm working on it now and the process code is a bit intricate. I'll let you know how it goes tomorrow after more work on it, but thus, far, I'm feeling pretty discouraged.

Also, as far as I can tell, there isn't a test suite that's narrowly testing the repo/queryable.ex file (I'm sure it's indirectly tested in other suites). How would you recommend I can test this so I can make sure any changes I make aren't breaking this for everyone else?

Also, is there a place where the ecto core team has listed the breaking changes from 2.x to 3.x for adapter authors/maintainers? It's been really, really discouraging to hit one undocumented breaking change after another. The list on the readme of adapter changes hasn't noted nearly any of the breaking changes I've run into. The blog post series on Platformatec also didn't address issues I ran into. I started a thread at Elixir Forum with some notes/questions about the experience which is how I discovered the PR that started this GH issue. https://elixirforum.com/t/resources-to-migrate-an-ecto-adapter-from-ecto-2-to-ecto-3/39288.

I know it's all open source and no one is being paid to maintain it, so I know there aren't any guarantees and I'm not expecting that! I really appreciate all the work the ecto team has done -- it's a great piece of software! My confusion comes from the difference between my experience with this adapter and my normal experience with Elixir. The Elixir docs are normally excellent and upgrades are smooth. My experience hasn't been smooth, so I'm assuming I'm probably not reading the right info or looking at the right mailing list. Any resources you could point me to would be really helpful! Thanks for all your do!

[josevalim]

Unfortunately there isn't much info. The adapter APIs have always been documented superficially because the best way to move forward was to copy the existing implementations. Perhaps that's a better route, use the existing Ecto v3.0 adapters as reference, extracting from the old one when necessary.

On the good news side, we don't plan any new major version of Ecto, so this should be the last time this happens.

---

Back to the issue here, I was also looking at the code after my comment and I think we assume a list fields in too many places. For example, even "select p" expands to a list of fields, so I think the :"$unknown" route may indeed work better. I just don't think which atom is reasonable. After all, :"$unknown" may still show up in user code. But that's a minor detail, make sure the actual atom is hidden in a function (such as Ecto.Adapter.Queryable.missing_data) and then we can change it easily.

[scottmessinger]

@josevalim Re: adapter API, that totally makes sense.

I think we assume a list fields in too many places.

I've been feeling the same! I was really struggling try to figure out how I'd do this.

"$unknown" route may indeed work better. I just don't think which atom is reasonable

Totally agree. Using a function would be a perfect fix.

I'll work on implementing that. I can dig through Ecto and find out where. If you have any tips or things to guide me, let me know! I'll be working on this for the next few days and try to get a PR up early next week.

[scottmessinger]

I think I found a way of doing this work entirely in the adapter. It appears the defaults are created when struct.__struct__() is called and not fetched per field. When I saw that, I wondered, "Can I just fetch the default from the struct in the adapter?" Turns out, I think I can.

In the execute function, I look for a struct in the query's from.source field. If it's there, I then instantiate the struct using struct.__struct__(). When the document is returned from the DB and processed in the adapter, I check if the field exists in the returned document. If the field doesn't exist in the returned document and there's a struct, I fetch the field from the struct which returns the default value.

Here's the relevant commit: commoncurriculum/mongodb_ecto_forked@d0b6c84

@josevalim Does this approach sound wise? Or, is there a pitfall or problem this could cause?

[josevalim]

@scottmessinger that's actually a good idea. Definitely 👍 from me. Just keep in mind that you can have fields from joins, so it is not always from from.source and not the schema (the module) may be nil too in schemaless queries. But then you can just use nil. Since all of them seem doable, I will go ahead and close this. Good call!

[scottmessinger]

@josevalim Awesome! Thanks so much for working through this with me -- I don't think I would have gotten there by myself.

Re: joins. That's good to know. I haven't heard of Mongo adding support for joins, but they did release support for transactions and validations, so who knows what comes next! I'll have to revisit this if Mongo adds joins.

And, as always, thanks for all your work on Elixir/Ecto!!!

[josevalim]

I don't think I would have gotten there by myself

my contribution to the final solution was 0.1%. :)