Improve generators attributes

[ShPakvel]

Fixes #5987

@chrismccord @josevalim, hi guys! I had some time and interest to focus on this topic. Fully understand and take risk proving these not small changes. It was pleasure to work on it for several weeks and learn more elixir and phoenix meanwhile. Hope you will share my enthusiasm about these changes, and it can become my 2 cents payback into OSS.

Example project

Here is example project to present changes for generators attributes. Each commit is generated code with specific options, covering cross cases for references.

Structural and general changes

• (1) Extract Mix.Phoenix.Attribute module. It parses cli attribute into Attribute struct, with validation of types and options, and prefilling some default options. Covered with tests.
• (2) Create specs for attribute types and options, with examples, to be used in generators docs and in console errors.
• (3) Extract Mix.Phoenix.Migration module. It prepares data, based on Attribute struct, to apply in migration file. Covered with tests. This eliminate migration data preparation in cases with no migration generation (e.g. for embedded schema or when flag is passed). Thoughts for later: potentially this logic can be used in new mix phx.gen.migration, as extension of mix echo.gen.migration with attributes.
• (4) Extract Mix.Phoenix.TestData module. It prepares data, based on Attribute struct, to apply in test files. Covered with tests.
• (5) Refactor schema related data preparation in Mix.Phoenix.Schema, based on Attribute struct. Only parsing cli attributes and preparing general sample_values is done during creation of Schema struct. Specific data for generated files are created on demand based on schema.attrs and schema.sample_values.
• (6) Rely on correct type inferred from referenced schema for associations, get rid of not needed any more general foreign keys type setup as schema module attribute. In original implementation there was a bug when referenced schema had different type of primary key then the one we specify to generating schema (binary_id case).
• (7) Extracted Mix.Phoenix.Web module. It prepares data, based on Attribute struct, to apply in html, live files. Covered with tests. It also postpone data generation. Thus e.g. data generation will be invoked only if conflicts check passes.
• (8) Add utility function Mix.Phoenix.indent_text to indent text or list of lines with spaces, and prepend and append empty lines when needed. Use it for new and old refactored cases.
• (9) Generate context files before json, html, live files, as it doesn't need specific extra bindings.
• (10) Extract fixture data generation, to be done only when fixture is going to be created.
• (11) Extract html assertion data generation, to be done only when web files is going to be created.
• (12) Correct array values rendering for index and html. Array of integers is interpolating into string with wrong representation ("#{[1,2]}" is <<1, 2>>, "#{[42,43]}" is "*+"). And it even leads to errors during run of generated tests (e.g. "#{[1042,1043]}" leads to error (ArgumentError) lists in Phoenix.HTML templates only support iodata, and not chardata.). Correct with simple default rendering logic - adjustment with (values || []) |> List.flatten() |> Enum.join(", "). It works for any levels nesting arrays. And feels reasonable default representation for array values.
• (13) In generated tests files, relocate create_attrs and update_attrs from module attributes into test body. More practical approach, as more often we need to generate some data. E.g. with current improvement we add generation of associated records, which is pretty general case (to have associations).

Attributes related changes

• (14) Fix [array,enum] issue. Potentially nested arrays like [array,[array,enum]] should work as well (for now it is outside of this change scope).
• (15) Add array alias for [array,string].
• (16) Fix possible collision of enum values with options. Extract enum values into specific options. Add support both variants: string option [one,two], integer option [[one,1],[two,2]]. Adjust migration and other files representation.
• (17) Add option-flag required to use in migration, schema and input form in html and live. When no attribute marked as required, the first given attribute is set to be required, with provided notification and confirmation message in console.
• (18) Add alias * for option-flag required.
• (19) Add option-flag virtual to use in schema and field skipping in migration. Add support of type any to be used only with this flag (add validation logic).
• (20) Add option-flag index to use in migration.
• (21) Add default,value option for types boolean, integer, decimal, float.
• (22) Add precision,value and scale,value options for decimal type, to use in migration and test files. Add validations: scale can be used only with precision, scale have to be less then precision, minimum for scale is 1, minimum for precision is 2.
• (23) Add option Context.Schema for references type. This options should be used when referenced schema cannot be inferred from the attribute name (e.g. when it has different context). Referenced table and type will be properly inferred from schema reflection. It should fix issue with incorrect references type in case when schema created --binary-id option, but referenced schema was created with general id. And vice versa.
• (24) Add option assoc,value for references type. For cases when it cannot be inferred from the attribute name.
• (25) Add option on_delete,value for references type. Pretty often we want to deviate from default value nothing.
• (26) Add option column,value for references type. For cases when referenced column differs from default id.
• (27) Add option type,value for references type. For rear cases when referenced schema is not reachable (e.g. in generators' tests), or when referenced column is not default id. For now support values id, binary_id, string.
• (28) Add option table,value for references type. For rear cases when referenced schema is not reachable (e.g. in generators' tests).
• (29) Update guides about changes in references interface and other parts.

Notes

UPD: The code is corrected to preserve old behavior for skipping default string type even when options is provided.
NOTE: (1) One drawback so far is necessity to specify string type if any option is provided. Which can be neglected I think. Otherwise we need more specific separation between name-type and options, something in a way of name:type|options. Then it would be easy and safe to parse omitted string type in name|options. Let me know if you want apply this approach. it should be easy enough to modify.

NOTE: (2) There is bug in current original version: live test fail in case of invalid attributes, when only boolean attributes exists (cannot find "can't be blank" text.). An this bug still exists in this PR. It is out of scope of this PR to decide approach to fix it.

NOTE: (3) Looks like there is issue with recently added --primary-key option (generated code has errors on execution "(Postgrex.Error) ERROR 23502 (not_null_violation) null value in column" - related to primary key column). An this bug still exists in this PR. It is out of scope of this PR to decide approach to fix it.

NOTE: (4) For references case we can also infer prefix option from reflection (similar to table and primary column and type). Which I will add if this PR improvements are welcomed. Decided to hold myself for now, before any conclusion about PR.

NOTE: (5) New attribute format uses some globbing characters: ,, [ and ]. Which works in bash. For zsh we can use simple alias mix='noglob mix', similar to rake in ruby community. I hope this PR's changes is good enough for some trade off.

[ShPakvel]

Corrected code to work for old elixir versions as well.

[josevalim]

Hi @ShPakvel, thank you for the pull request. However, this pull request merges too many changes at once, it includes refactoring, new features, and bug fixes. This makes it hard to review and hard for us to guarantee there are no new regressions, because code and tests change at the same time. In practice, each of those should be done in individual separate PRs, probably starting with bug fixes. Would you like to break this apart? First by submitting individual PRs for the bug fixes? I know it is a big ask, so feel free to decline.

[ShPakvel]

Hi @josevalim. I am sorry. Right now I am indeed lack of time and energy to rewrite this all to gradual updates. Thus I have to decline it, thank you for understanding it upfront. 🙇🏼

I generally work with small gradual fixes, as can be seen in my previous PRs. Totally understand the difficulty of reviewing it, as I'm doing reviews myself all the time.

Not to push anything, just couple notes in defense of this big change:

• I started with particular fixes per type and option, and came to conclusion that I described in the issue - original simple format for attributes have been already outgrown for some time, being the main reason for most of complexity and interlacing among types and options. I saw clear way to resolve all of it via new format (adding just ,, [ and ]). New format led to more independent and robust parsing. What led to unified usage of parsed %Attribute() in most of the files. Some parts could be separated and postponed though, of course 😞 . I took the risk to present solid foundation for future enhancements, hoping that quality of the code will be good enough for not too difficult review.
• Main changes is done in new modules lib/mix/phoenix/{attribute,migration,test_data,web}.ex and simplified schema there. It is thoroughly covered with tests, not only check specific positive or negative cases, but strict comparison of whole text generation - thus checking that only what is expected is generated and nothing else.
• I also added example project with quite big coverage of types and options. Just updated it to have not only html, but also live and json. In readme I've provided log of generators and collapsed it for quick grasp of each case. 5 generator runs (commits) per generator, which cover parent-child relation with the same and different context and binary_id application, array, enum, etc.

With all of it, I know that this review is not easy one, and possibilities of not considered issues (regression). Sorry for this. Not expecting review to be quick either. Especially knowing that you guys busy with elixir upgrade this time. I just hope this update will be considered as promising one. I will try to find time to contribute further, with back to small gradual changes 😅.

In any case, always thank you guys for your work and time. 🙇🏼 🍵