pow-auth / pow_assent

Multi-provider authentication for your Pow enabled app
https://powauth.com
MIT License
318 stars 50 forks source link

Remove invalid schema attr #220

Closed leandrocp closed 2 years ago

leandrocp commented 2 years ago

On Ecto v3.8.0 I started getting the following error:

== Compilation error in file lib/my_app/accounts/user_identity.ex ==
** (ArgumentError) invalid option :null for field/3
    (ecto 3.8.0) lib/ecto/schema.ex:2214: Ecto.Schema.check_options!/3
    (ecto 3.8.0) lib/ecto/schema.ex:1911: Ecto.Schema.__field__/4
    (elixir 1.13.4) lib/enum.ex:937: Enum."-each/2-lists^foreach/1-0-"/2
    lib/my_app/accounts/user_identity.ex:8: (module)
    (elixir 1.13.4) lib/kernel/parallel_compiler.ex:346: anonymous fn/5 in Kernel.ParallelCompiler.spawn_workers/7

Which makes sense because null is not a valid attr (seems like it has never been), see https://github.com/elixir-ecto/ecto/blob/4fa2c26dc2064aa2963b96bddee662d18a63e7fd/lib/ecto/schema.ex#L491-L507

I just don't know why it started failing on Ecto v3.8.0 and not on previous versions, but either way seems like that null: false should be removed.

Tested on Elixir v1.13.0 and v1.13.4

danschultzer commented 2 years ago

Thanks! I've released v0.4.13 to resolve this. In ecto_sql 3.8.0 the options for fields are now validated. We still need the migration columns to have NOT NULL constraint set to ensure data integrity. I've separated the two options so they are dealt with appropriately, and this makes it trivial to add any options in the future if necessary.

Let me know if there's any issues with this release!

leandrocp commented 2 years ago

Thanks! I've released v0.4.13 to resolve this. In ecto_sql 3.8.0 the options for fields are now validated. We still need the migration columns to have NOT NULL constraint set to ensure data integrity. I've separated the two options so they are dealt with appropriately, and this makes it trivial to add any options in the future if necessary.

Let me know if there's any issues with this release!

Thanks! I didn't realize attrs were used by migrations too but it makes sense. I'll give it a try and let you know if something doesn't work as expected.