ash icon indicating copy to clipboard operation
ash copied to clipboard

fix: dont validate constraints in union_types

Open zackattackz opened this issue 1 month ago • 1 comments

Remove call to Spark.Options.validate!/2 inside Ash.Type.Union.union_types/1. It is unnecessary and it causes an issue when attempting to define unions using NewTypes whose parents have required: true in their constraints.

Add in two test cases in the union type's tests:

  1. Checks that we can define and use a union with the mentioned definition.
  2. Ensures that nothing was lost when we removed the call to Spark.Options.validate!/2 by demonstrating that the constraints are still checked when defining a resource using a union with the mentioned definition.

These tests fail to compile/run without the fix.

Fixes https://github.com/ash-project/ash/issues/2468


Extra notes

I believe removing this call to validate! is fine. However it did used to modify the constraints by adding in default values, e.g if type was Ash.Type.String, the call to validate! would return constraints with [trim?: true, allow_empty?: false] before adding them to the final result from union_types. But this seems to not be necessary as when the type is fully initialized it will include those constraints anyways. (I could add a test case to show this if wanted)

The problem with keeping the validate! call, is that with NewTypes validate! is expecting to receive their applied constraints as well. I think we could do this by calling Ash.Type.init/2 like so:

schema = Ash.Type.constraints(type)

{:ok, constraints} = Ash.Type.init(type, config[:constraints] || [])
constraints = Spark.Options.validate!(constraints, schema)

{key, config |> Keyword.put(:constraints, constraints) |> Keyword.put(:type, type)}

But I think it'd just be more preferable to remove the validate! call altogether since it seems unnecessary.

I may be missing some considerations there though, so please scrutinize this!

Contributor checklist

Leave anything that you believe does not apply unchecked.

  • [x] I accept the AI Policy, or AI was not used in the creation of this PR.
  • [x] Bug fixes include regression tests
  • [ ] Chores
  • [ ] Documentation changes
  • [ ] Features include unit/acceptance tests
  • [ ] Refactoring
  • [ ] Update dependencies

zackattackz avatar Dec 12 '25 18:12 zackattackz

I may amend to add a regression test case for the part I mentioned in extra notes regarding the defaults being added to constraints

zackattackz avatar Dec 12 '25 18:12 zackattackz

Could you try cloning ash_graphql down and pointing its ash dependency at this branch and running tests? If they pass then I think we're good here.

zachdaniel avatar Dec 23 '25 00:12 zachdaniel

Force pushed to rebase over latest main branch, also added in that extra regression test I mentioned before (check that a type with a default constraint gets the default value applied properly)

So then I ran the tests in ash_graphql and it seems to work.

For sanity check, I ran mix check from ash_graphql @ f86cd51 without any changes. It yields the logs attached in ash_graphql-mix-check-before.log

Then in mix.exs I changed the ash dependency to point to my local repo, which was at this branch:

 defp deps do
    [
      # {:ash, ash_version("~> 3.5 and >= 3.5.13")},
      {:ash, path: "../ash"},
      ...

And ran mix deps.get, producing a new mix.lock (had to rename to .txt to attach)

Finally ran mix check again, yielding this successful output ash_graphql-mix-check-after.log

zackattackz avatar Dec 23 '25 18:12 zackattackz

🚀 Thank you for your contribution! 🚀

zachdaniel avatar Dec 24 '25 09:12 zachdaniel