Skip to content

Conversation

@mikaelweiss
Copy link

Contributor checklist

Leave anything that you believe does not apply unchecked.

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

Resolves #365

@zachdaniel
Copy link
Contributor

Very nice! I think we'll need to do a few more things on this one.

Firstly, we'll need to make sure that the callbacks defined in the EctoType module for a given type still follow Ecto's expectations:

https://github.com/elixir-ecto/ecto/blob/v3.13.5/lib/ecto/type.ex#L294

And to really vet that, we'll want to make sure to test ash_postgres against these changes. A good way to do that will be to clone down ash_postgres, set export ASH_VERSION=local and then run the tests (typically mix test --exclude migration).

@mikaelweiss
Copy link
Author

mikaelweiss commented Feb 11, 2026

The load/3 and dump/3 functions in the EctoType module (lib/ash/type/type.ex:1731 and lib/ash/type/type.ex:1744) both swallow errors with catch-all clauses, which follows Ecto's expectations:

@impl true
def load(term, _, params) do
  parent = @parent

  case parent.cast_stored(term, params) do
    {:ok, value} ->
      {:ok, value}

    _ ->
      :error
  end
end

@impl true
def dump(term, _dumper, params) do
  parent = @parent

  case parent.dump_to_native(term, params) do
    {:ok, value} ->
      {:ok, value}

    _ ->
      :error
  end
end

I also ran ASH_VERSION=local mix test in ash_postgres (full test suite to be sure) and they passed.

Is there anything else you'd like me to test or check?

@zachdaniel
Copy link
Contributor

Ah, yeah so there is one more major thing to test. What we want to do is set up a resource that works against ETS, manipulate the ETS table to have a value that can't be loaded, and ensure that the error bubbles up in a way that is not exposed to the end user. It will likely need a custom error built to function that way.

Both dump_to_native and cast_stored should produce errors different from InvalidAttribute or InvalidArgument for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support error messages in cast_stored and dump_to_native.

2 participants