Is "Simplifying Ecto schema fields validation" really an improvement?

[RudolfMan]

First of all, thank you for this work! I really appreciate it!

I wanted to open a discussion regarding "Simplifying Ecto schema fields validation".

I've seen developers, when learned about Ecto schema reflection, start use it in changeset function just like shown in the refactoring example.

I'd argue it's a bad practice that potentially opens up security issues akin to what we have seen in Rails community prior to the introduction of StrongParameters.

And in real world a better practice, IMO, would be to explicitly declare separate lists for permitted_fields and required_fields. Either inline or in a variable or a module attribute, the point is to be explicit about fields.

Moreover, different actions might require different set of permitted_fields and required_field. Which might lead us to create different changeset functions, like: create_changeset that would allow and require more fields, while for update_changeset it could be only a subset of fields required or even allowed. But that's slightly tangental, though related.

Surely one could oppose saying that the developer must know what they are doing, instead of blindly copying the pattern from the internet. But these sort of "lists of refactorings" just like "anti-patterns" might looks like a "blessed way", so we gotta be careful 🙂

[lucasvegi]

Hello @RudolfMan,

First of all, thank you very much for opening this issue and raising these points. This kind of discussion allows us to clarify some important points, which often lead to doubts or distorted interpretations.

The catalog of refactorings that we are proposing, much like Martin Fowler's traditional refactoring catalog, does not aim to be a "blessed way" or establish rules/requirements such as:

"The before code is always bad and you should replace all uses of it with the after."

The purpose of the catalog is simply to offer alternatives to developers, which may not always be the best for certain contexts. Developers must analyze the trade-offs involved to decide if the refactoring makes sense for their context. Let's say the catalog, instead of establishing a rule, asks the developer something like:

"Have you considered writing things like this? It might not work, and it's fine if you leave your code as-is."

To illustrate how catalogs do not intend to establish quality rules, there are some studies investigating developers' motivations for refactoring their code (e.g., https://doi.org/10.48550/arXiv.2007.02194 and https://doi.org/10.1145/2950290.2950305), and the removal of code smells (low-quality code) is just one of them.

Staying on topic and speaking more specifically about the Simplifying Ecto schema fields validation refactoring, determining whether it improves quality or not, or whether it should be applied or not, all depends on the context. I completely agree with you that a code that explicitly lists all fields, as shown below, provides more visibility to the developer about what is being validated, which is a very positive aspect:

# Before refactoring:

def changeset(attrs) do
  # Manual listing of schema fields
  schema_fields = [:carrier_time, :carrier_date, :carrier_name, :carrier_number, :carrier_terminal, :carrier_type]

  %__MODULE__{}
  |> cast(attrs, schema_fields)
  |> validate_required(schema_fields, message: "Missing Field")
end

However, for schemas with a large number of fields, implementing like this can lead to longer code (time-consuming to implement) and prone to human typing errors. This is not necessarily a problem, but a developer may choose to do it differently.

As for the necessity to create subsets of fields, that would also be possible with the refactored version of the catalog. For example:

non_required_fields = [:carrier_time, :carrier_date]
required_fields = __schema__(:fields) -- non_required_fields

Assuming a hypothetical scenario where the quantity of non_required_fields is equal to or greater than required_fields, the developer may eventually choose not to use __schema__/1, mainly because it would have little or no benefit.

This was just an example to make it clear that this, and no other refactoring, should be taken as a rule. Everything depends on the context and the trade-offs involved in it.

Feel free to continue the conversation and raise other points. Again, thank you very much for the issue!