Add :source option for schema `field` #1998

Bharat311 · 2017-03-20T11:26:47Z

This PR adds a source option to field.

The source name is then used within the database, whereas the name provided with field is used to refer to that field in the codebase.

Refer #1892 for more details.

TODO:

sourcelevel-bot · 2017-03-20T11:26:54Z

Hello, @Bharat311! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

Bharat311 · 2017-03-20T11:44:21Z

Hi, @josevalim - Could you please review the work done so far in this PR and let me know your thoughts ?

Also, I would like to add the ability to query a record using an aliased name. So, If the schema is

defmodule EctoDemo.Post do
  use EctoDemo.Web, :model

  schema "posts" do
    field :title, :string
    field :b, :string, alias: :body
  end
end

then

EctoDemo.Post |> where(body: "some test body") |> Repo.all

should be perfectly valid statement and return all posts with body="some test body".
Same goes for other query constructs like select, order, group, etc.

I am a bit confused as to what will be the best place to replace the aliased names with the actual names - The Ecto.Query.Builder or the Ecto.Query.Planner.

I believe it should be done in Ecto.Query.Planner because we prepare the final query at this place and also have all the related schema info, but i am unsure as to how it could be done, as it already receives the quoted expression in the %Ecto.Query{} for various expressions like select, where, order, etc. set by the Ecto.Query.Builder and i don't know whether / how we could modify it.

OvermindDL1 · 2017-03-20T15:04:35Z

Oh please bring something in like this! I keep having to write things like from s in sldbn, where: s.somelongdatabasename_id = 42 and s.somelongdatabasename_blorp = ^vwoop because I have to interact with an old database that prefix's every-single-field-with-the-table-name (which is not terribly short on many of them either); it is so verbose.

So yes, I have a use-case for it. Thanks for the work!

sourcelevel-bot · 2017-03-21T10:38:11Z

lib/ecto/schema.ex

@@ -396,13 +398,14 @@ defmodule Ecto.Schema do
      autogenerate = @ecto_autogenerate |> Enum.reverse
      autoupdate = @ecto_autoupdate |> Enum.reverse
      fields = @ecto_fields |> Enum.reverse
+      aliased_fields = @ecto_aliased_fields |> Enum.reverse


Use a function call when a pipeline is only one function long

Bharat311 · 2017-03-23T06:12:32Z

Hi @josevalim. Did you manage to get a look at this PR and my earlier comment ?

Apologies if i seem too impatient, but i really need to complete this PR to completely switch over my Rails project to an Elixir app and i am stuck with the above mentioned problem. Any help in this regard is highly appreciated.

Thanks a ton for all your efforts :)

michalmuskala · 2017-03-23T08:05:24Z

I did have a look, and I'm not sure that's the way to do it. We introduce quite a lot of additional reflection lookups with this, and I'm not sure we're doing the conversions in the right places.

The conversions should happen so that the user never sees the database columns, and that the database never sees the user columns.

Bharat311 · 2017-03-23T08:29:38Z

@michalmuskala - Thanks for your valuable feedback. I thought that it would be better if the user would be able to use both the aliased name and the original name for the field.

However, what you suggest also makes sense. But, I am not sure as to how i could implement it.
If you could guide me, I'd be happy to make the necessary changes to this PR.

Thanks again for your feedback.

johnnyshields · 2017-03-27T06:13:36Z

@michalmuskala @josevalim please provide guidance to Bharat on this PR?

IMHO it's better if the Ecto user can use the alias/non-alias names interchangeably, while the DB only sees the aliased name (if present) -- this is what Ruby's Mongoid lib allows for example.

ericmj · 2017-03-28T07:40:42Z

IMHO it's better if the Ecto user can use the alias/non-alias names interchangeably, while the DB only sees the aliased name (if present) -- this is what Ruby's Mongoid lib allows for example.

Can you elaborate on why this is better? The problem is that there is only one way to name struct keys so you have a mismatch between your ecto queries/changsets etc. and your elixir code.

wojtekmach · 2017-03-28T07:47:53Z

maybe instead of:

field :first_name, alias: :fn

we should have:

field :fn, as: :first_name

which implies 1-to-1 conversion?

johnnyshields · 2017-03-28T10:18:47Z

@ericmj

Can you elaborate on why (using alias/non-alias names interchangeably) is better?

EDIT - Changed my opinion. It's not needed. Even though my current library (Ruby Mongoid) supports it, I've never actually used it.

@wojtekmach

we should have (as: parameter)

I think store_as: or db: might be a better name. The word "store" makes it clear that it's the DB name (rather than as: / :alias: which are ambiguous), and also we can re-use the same :store_as option for embedded models, e.g. embeds_many :addresses, store_as: :addrs

johnnyshields · 2017-03-28T11:37:13Z

@ericmj edited above

johnnyshields · 2017-03-30T07:27:16Z

hi all, may we please get resolution on the points above so we can move this PR forward? we need it for our project.

raarts · 2017-04-05T19:56:24Z

My 0.02. For users of the module the name of the table is determined by the module name. The schema macro then points to the actual underlying table.

In a similar way the users of the module will expect the first argument to the field macro to be the field name, and will expect to be able to use that name in their code.

So my preference would be something like

field :first_name, xxxxx: :fn

On the other hand the alias name now introduces confusion, because that is something you should use instead of something else.

So my suggestion would be:

field :first_name, column: :fn

or even better:

field :first_name, source: :fn

because the docs for the schema macro also use source for the underlying table.

Bharat311 · 2017-04-06T08:40:27Z

@michalmuskala

I did have a look, and I'm not sure that's the way to do it. We introduce quite a lot of additional reflection lookups with this, and I'm not sure we're doing the conversions in the right places.

Could you please guide me to the right places where these conversions should take place. I believe it to be the Ecto.Query.Planner ?

The conversions should happen so that the user never sees the database columns, and that the database never sees the user columns.

I agree and would make the necessary changes 👍

johnnyshields · 2017-04-10T13:03:16Z

Hi all, just want to confirm the name of option we're adding. Candidates are:

as:
alias:
store_as:
db:
db_name:
repo:
repo_name:
column:
source:

I prefer store_as: or db_name: @josevalim @michalmuskala any strong opinions here?

OvermindDL1 · 2017-04-10T14:11:25Z

As a vote, I also prefer something like :store_as or :db_column_name or something that is very clear.

ericmj · 2017-04-10T14:14:09Z

I prefer :source since that's what we use for the table name with schemas.

overture8 · 2017-04-10T18:00:36Z

I like :store_as or :source. :db_column_name is a bit too long and :db_name is potentially ambiguous.

sourcelevel-bot · 2017-04-14T11:06:32Z

lib/ecto/query/planner.ex

+
+    %{wheres: wheres, havings: havings} = query
+
+    query = %{ query | wheres: rewrite_aliases(wheres, aliases_map),


There is no whitespace around parentheses/brackets most of the time, but here there is.

sourcelevel-bot · 2017-04-14T11:06:32Z

lib/ecto/query/planner.ex

+    %{wheres: wheres, havings: havings} = query
+
+    query = %{ query | wheres: rewrite_aliases(wheres, aliases_map),
+                       havings: rewrite_aliases(havings, aliases_map) }


There is no whitespace around parentheses/brackets most of the time, but here there is.

sourcelevel-bot · 2017-04-14T11:06:32Z

lib/ecto/query/planner.ex

+
+    field_name = Enum.at(aliases_map, binding)[field] || field
+
+    %{query | expr: {op, [], [{{:., [], [{:&, [], [binding]}, field_name]}, [], []}, rhs]} }


There is no whitespace around parentheses/brackets most of the time, but here there is.

sourcelevel-bot · 2017-04-14T11:06:32Z

lib/ecto/query/planner.ex

+    %{query | expr: {op, [], [{{:., [], [{:&, [], [binding]}, field_name]}, [], []}, rhs]} }
+  end
+
+  defp aliases_list(%{ sources: sources }) do


There is no whitespace around parentheses/brackets most of the time, but here there is.

johnnyshields · 2017-04-14T14:15:26Z

lib/ecto/schema.ex

+      def __schema__(:source),          do: unquote(source)
+      def __schema__(:fields),          do: unquote(field_names)
+      def __schema__(:db_fields),       do: unquote(db_field_names)
+      def __schema__(:aliased_fields),  do: unquote(aliased_fields)


seems unnecessary to store both db_fields and aliased fields ?

johnnyshields · 2017-04-14T14:16:45Z

lib/ecto/query/planner.ex

@@ -578,6 +579,33 @@ defmodule Ecto.Query.Planner do
    end
  end

+  defp rewrite_aliases(query) do


how about calling this "evolve_field_names"

johnnyshields · 2017-05-10T14:07:06Z

@michalmuskala @josevalim PR is ready for merge, please review.

johnnyshields · 2017-06-11T19:43:36Z

@michalmuskala @josevalim pinging again on this, been a month and no movement.

hbobenicio · 2017-06-15T02:43:21Z

+1 on this.
Really looking forward for this feature.
Pretty common case when database column name conventions are not desired to be inherited on our ecto models.

zachdaniel · 2017-07-12T18:30:31Z

We'd love to see this. And even better if it could support fragments :D

johnnyshields · 2017-07-15T03:30:18Z

@josevalim @michalmuskala this pr is now conflicted. If we resolve conflicts will you merge?

josevalim · 2017-07-15T07:37:03Z

We can rebase when merging, no worries.

johnnyshields · 2017-07-17T15:53:10Z

@josevalim so how do we move forward here? This PR has been collecting dust for several months now.

josevalim · 2017-07-17T16:41:53Z

If you need to use it now, feel free to use the branch. It will also help with finding any bugs. This will be merged into Ecto when we have time to fully review it. It may be in a week or it may be in two months. -- *José Valimwww.plataformatec.com.br <http://www.plataformatec.com.br/>Founder and Director of R&D*

johnnyshields · 2017-07-17T17:51:14Z

@josevalim my company wrote this (@Bharat311 works for us and wrote it on company time) and we are using this branch on 3 Elixir projects currently.

josevalim · 2017-07-17T18:05:35Z

Even better! Thank you! <3 -- *José Valimwww.plataformatec.com.br <http://www.plataformatec.com.br/>Founder and Director of R&D*

mpinkston · 2017-07-24T01:37:35Z

I'd like to add my 👍 here. I've been using the branch successfully and have come to depend on it (but master has since added some critical updates as well)

johnnyshields · 2017-07-24T02:40:38Z

@Bharat311 please update this branch with master if possible.

…t, updates, etc.

Bharat311 · 2017-07-25T12:40:57Z

@johnnyshields @mpinkston - I have rebased the changes to latest master.

paulnicholson · 2017-07-25T15:10:03Z

I would also like to see this merged, needed to use it today.

johnnyshields · 2017-07-26T01:36:28Z

@josevalim @michalmuskala please help with this

michalmuskala · 2017-07-26T09:29:11Z

lib/ecto/changeset.ex

+  defp get_field_source(%{data: %{__struct__: schema}}, field) when is_atom(schema),
+    do: schema.__schema__(:source, field) || field
+  defp get_field_source(%{data: data}, _field),
+    do: raise(ArgumentError, "cannot add constraint to changeset because it does not have a source, got: #{inspect data}")


This is a slight backwards incompatibility. Previously we would just use the field but now we'll fail. I think we should default to using the field in case there's no schema.

@michalmuskala - Updated the PR.

michalmuskala · 2017-07-26T09:32:19Z

lib/ecto/query/planner.ex

@@ -215,6 +215,7 @@ defmodule Ecto.Query.Planner do
    query
    |> prepare_sources(adapter)
    |> prepare_assocs
+    |> rewrite_aliases


This is going to have a performance impact since it means we'll need to traverse the whole query every time. Would it make sense to do it only if there is something to rewrite? Alternatively, maybe we could do some of the work in the builder?

@michalmuskala - I am new to elixir and am not familiar with internal workings of ecto. I am not too sure if / how we could do this in builder. I, however, think that it would be better to traverse only if the schema uses field aliases and needs rewrite. I think this should be doable and would work on it.

michalmuskala · 2017-07-26T09:34:23Z

lib/ecto/repo/schema.ex

@@ -706,7 +706,7 @@ defmodule Ecto.Repo.Schema do
  defp dump_field!(action, schema, field, type, value, adapter) do
    case Ecto.Type.adapter_dump(adapter, type, value) do
      {:ok, value} ->
-        {field, value}
+        {schema.__schema__(:source, field), value}


Since we'll do this for all fields anyway, would it make sense to load all the "sources" as a map and fetch from the map similar to what we do for types?

@michalmuskala - If i understand it correctly, the current types are stored in the changeset. So, we should also add a key source in the changeset similar to types and read the source from there.

This happens at the point we no longer use changesets, so I don't see a reason to store it in the changeset.

michalmuskala · 2017-07-26T10:12:15Z

lib/ecto/schema.ex

@@ -255,6 +255,8 @@ defmodule Ecto.Schema do
  * `__schema__(:primary_key)` - Returns a list of primary key fields (empty if there is none);

  * `__schema__(:fields)` - Returns a list of all non-virtual field names;
+  * `__schema__(:db_fields)` - Returns a list of all field names as stored in db;
+  * `__schema__(:aliases)` - Returns a list of fields with their changed source;


I'm not sure I like the name aliases and source we introduce. The sound confusing and inconsistent. Maybe we should go for field_alias and field_aliases?

@michalmuskala - I agree field_aliases is better than aliases, I'll rename the method. But i am confused about the field_alias. Do you want me to replace the option :source ?

I have intentionally kept it as source because we use it for defining name of the schema as well. I think we could better it with field_source. (You can check the discussions it the same PR for other suggestions as well)

Moreover, i think field_alias would also require the following change. Currently,

field :first_name, :string, source: :fn

will become:

field :fn, :string, field_alias: :first_name

Since this is something big, I wanted to know your thoughts on this.

👍 for using consistent nomenclature everywhere

I was referring only to what we use in the reflection. I would avoid source exactly because it's already quite an overloaded term and is used primarily to refer to the table name. It might be fine within the limited scope of field macro, but I think it's confusing for the reflection mechanism since you have much less context there.

Okay. I get it now. I have made the necessary changes. 👍

I haven't changed the option name, its still called as :source. However, within the code everywhere, we are now using field_alias or field_aliases.

I disagree with this, because the column name is not an alias for our field, it's the other way around. I would suggest using something like field_column_name or field_source internally.

I agree with @raarts. field_source is probably better to account for NoSQL dbs which don't have columns in the strict sense.

michalmuskala · 2017-07-26T10:12:54Z

lib/ecto/schema.ex

-      they would then be the same for all records
+      they would then be the same for all records.
+
+    * `:source` - Defines the name that is to be used in database for this field.


Should source be a string or an atom? I think an atom would be better.

@michalmuskala - Yes, I agree. The source should be defined as an atom. However, I am not able to understand what needs change as per this comment?

michalmuskala · 2017-07-26T10:14:10Z

lib/ecto/schema.ex

  @doc false
  def __load__(schema, prefix, source, context, data, loader) do
    struct = schema.__struct__()
    types = schema.__schema__(:types)
+    aliases = schema.__schema__(:aliases)
+
+    data = replace_field_sources(data, aliases)


This adds another iteration over the data on every load. It will have a significant impact on performance. Is there maybe a way to do this in one pass?

@michalmuskala - I understand that this affects performance but i am not sure if there is any other better way to do this. If you could suggest me something better, I'll gladly change it.

The primary problem for me is that this affects performance for everybody - no matter if you use the feature or not.

@michalmuskala - I understand your concern and agree with it completely. However, what i meant was that i can't think of any other way to fix this. If you could help me in the right direction, i'll make the necessary changes.

Another option that i can think of is providing a global configuration that can enable / disable this feature. However, again, i am not sure if it would help.

In production load would only be called once at load time, correct?

No load is called every time we load data from the database - it's responsible for putting the raw data into schemas with proper types.

I will think on how we could make this less costly

michalmuskala · 2017-07-26T10:15:15Z

lib/ecto/schema.ex

    field_names = Enum.map(fields, &elem(&1, 0))

+    db_field_names = Enum.map(field_names, fn(field) ->
+      if List.keymember?(aliased_fields, field, 0), do: aliased_fields[field], else: field


Wouldn't be the same as Keyword.get(aliased_fields, field, field)?

Yes, this should work fine. Updated the PR. 👍

Bharat311 · 2017-07-28T08:36:36Z

@michalmuskala - Thanks for the review. I'll look over these points and will update the PR shortly.

sourcelevel-bot · 2017-07-31T08:28:03Z

Ebert has finished reviewing this Pull Request and has found:

19 possible new issues (including those that may have been commented here).

But beware that this branch is 7 commits behind the elixir-ecto:master branch, and a review of an up to date branch would produce more accurate results.

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

josevalim · 2017-08-04T21:39:56Z

I will merge this pull request over the next days. Thanks for all the work and please do not rebase!

Notes to self:

There are no integration tests
Returning in insert_all and insert is not considered in field aliases

josevalim · 2017-08-05T14:25:09Z

Merged manually and pushed it to master with further fixes, thank you! ❤️ 💚 💙 💛 💜

johnnyshields · 2017-08-05T16:12:22Z

@josevalim thank you so much for your help, and great work @Bharat311

Bharat311 force-pushed the f-alias branch from cf329f3 to c2a13ff Compare March 21, 2017 10:27

sourcelevel-bot bot reviewed Mar 21, 2017

View reviewed changes

sourcelevel-bot bot reviewed Apr 14, 2017

View reviewed changes

Bharat311 force-pushed the f-alias branch from 8c34580 to ce4f655 Compare April 14, 2017 11:44

johnnyshields reviewed Apr 14, 2017

View reviewed changes

josevalim force-pushed the master branch 3 times, most recently from 5387818 to 7acc780 Compare April 23, 2017 09:26

johnnyshields mentioned this pull request Jun 15, 2017

Allow schema :field to have an alias #1892

Closed

Bharat311 added 4 commits July 25, 2017 18:00

Add :source option for fields and embedded schemas

9be47a4

rewrite_aliases for remaining sql queries like order, select, distinc…

ef0635f

…t, updates, etc.

Fix failing tests

40ab92f

Add tests for the changes done

828c6b6

Bharat311 force-pushed the f-alias branch from 55e5c1f to 828c6b6 Compare July 25, 2017 12:36

michalmuskala requested changes Jul 26, 2017

View reviewed changes

Requested changes

2b97046

Rename aliases -> field_aliases, source -> field_alias

b6f5f36

josevalim closed this Aug 5, 2017


		%{wheres: wheres, havings: havings} = query

		query = %{ query \| wheres: rewrite_aliases(wheres, aliases_map),


		field_name = Enum.at(aliases_map, binding)[field] \|\| field

		%{query \| expr: {op, [], [{{:., [], [{:&, [], [binding]}, field_name]}, [], []}, rhs]} }

Add :source option for schema field #1998

Add :source option for schema field #1998

Conversation

Bharat311 commented Mar 20, 2017 • edited Loading

TODO:

sourcelevel-bot bot commented Mar 20, 2017

Bharat311 commented Mar 20, 2017

OvermindDL1 commented Mar 20, 2017

sourcelevel-bot bot Mar 21, 2017

Choose a reason for hiding this comment

Bharat311 commented Mar 23, 2017

michalmuskala commented Mar 23, 2017

Bharat311 commented Mar 23, 2017

johnnyshields commented Mar 27, 2017 • edited Loading

ericmj commented Mar 28, 2017

wojtekmach commented Mar 28, 2017

johnnyshields commented Mar 28, 2017 • edited Loading

johnnyshields commented Mar 28, 2017

johnnyshields commented Mar 30, 2017

raarts commented Apr 5, 2017 • edited Loading

Bharat311 commented Apr 6, 2017 • edited Loading

johnnyshields commented Apr 10, 2017

OvermindDL1 commented Apr 10, 2017

ericmj commented Apr 10, 2017

overture8 commented Apr 10, 2017

sourcelevel-bot bot Apr 14, 2017

Choose a reason for hiding this comment

sourcelevel-bot bot Apr 14, 2017

Choose a reason for hiding this comment

sourcelevel-bot bot Apr 14, 2017

Choose a reason for hiding this comment

sourcelevel-bot bot Apr 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnnyshields commented May 10, 2017

johnnyshields commented Jun 11, 2017

hbobenicio commented Jun 15, 2017

zachdaniel commented Jul 12, 2017

johnnyshields commented Jul 15, 2017

josevalim commented Jul 15, 2017

johnnyshields commented Jul 17, 2017

josevalim commented Jul 17, 2017 via email

johnnyshields commented Jul 17, 2017 • edited Loading

josevalim commented Jul 17, 2017 via email

mpinkston commented Jul 24, 2017

johnnyshields commented Jul 24, 2017

Bharat311 commented Jul 25, 2017

paulnicholson commented Jul 25, 2017

johnnyshields commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bharat311 Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bharat311 commented Jul 28, 2017

sourcelevel-bot bot commented Jul 31, 2017

josevalim commented Aug 4, 2017

josevalim commented Aug 5, 2017

Add :source option for schema `field` #1998

Add :source option for schema `field` #1998

Bharat311 commented Mar 20, 2017 •

edited

Loading

johnnyshields commented Mar 27, 2017 •

edited

Loading

johnnyshields commented Mar 28, 2017 •

edited

Loading

raarts commented Apr 5, 2017 •

edited

Loading

Bharat311 commented Apr 6, 2017 •

edited

Loading

johnnyshields commented Jul 17, 2017 •

edited

Loading

Bharat311 Jul 31, 2017 •

edited

Loading