Add mix ecto.query task#4731
Conversation
Refs elixir-ecto#4719 - adds mix ecto.query to evaluate an Ecto query expression, run it through the selected/default repo inside a read-only transaction, and print schema level data.
| defmodule Mix.Tasks.Ecto.Query.Schema do | ||
| @moduledoc false | ||
|
|
||
| defstruct [:schema, :fields] | ||
| end |
There was a problem hiding this comment.
Why does this struct need to exist? I think it would probably be nicer to rely on the existing inspect implementation for the structs...
There was a problem hiding this comment.
This is only there to keep schema shaped output after dropping __meta__, redacted fields, and association fields. Without it, the cleaned value becomes a map and loses the schema name.
I found the SchemaName{...} shape easier to read when testing, but I can simplify to maps or regular inspect output if preferred.
| defp clean_entry(%{__struct__: schema} = struct) do | ||
| if function_exported?(schema, :__schema__, 1) do | ||
| drop_fields = [ | ||
| :__meta__ | schema.__schema__(:associations) ++ schema.__schema__(:redact_fields) |
There was a problem hiding this comment.
I wonder if associations should only be deleted if they aren't preloaded
There was a problem hiding this comment.
That's a good point. I interpreted the issue as dropping all association fields, but keeping preloaded associations is more useful. I’ll change it to keep preloaded associations.
There was a problem hiding this comment.
Inspect has a default_inspect_fun. I wonder if we can override it during the execution of this task, so we don't have to traverse everything and create a fake Mix.Tasks.Ecto.Query.Schema. Basically we intercept all structs with a __meta__ field pointing to Ecto, otherwise we render them as is.
There was a problem hiding this comment.
I'll look into overriding default_inspect_fun for this
|
|
||
| If the `:read_only` option is true, the adapter must run the transaction | ||
| in read-only mode or raise if read-only transactions are not supported. |
There was a problem hiding this comment.
I like this option conceptually, but the need to add it makes me wonder if the whole thing should be in ecto_sql 🤔
There was a problem hiding this comment.
Yeah, I had the same concern. I started here because the command evaluates an Ecto query and calls Repo.all, so the schema-level output felt like the more natural first pass.
The read_only: true part is the bit that crosses into adapter behavior. My thinking was to document and pass the option through in Ecto, then handle the actual SQL adapter support in an ecto_sql follow-up, together with the --sql option.
There was a problem hiding this comment.
Agreed on moving it to ecto_sql!
There was a problem hiding this comment.
Thanks, in that case I'll close this PR and handle this in ecto_sql
|
to be handled in |
3 participants
Refs #4719