Skip to content

fix: handle ForbiddenField in full_diff when using field_policies (#215)#219

Open
M-Gonzalo wants to merge 4 commits intoash-project:mainfrom
M-Gonzalo:main
Open

fix: handle ForbiddenField in full_diff when using field_policies (#215)#219
M-Gonzalo wants to merge 4 commits intoash-project:mainfrom
M-Gonzalo:main

Conversation

@M-Gonzalo
Copy link
Copy Markdown

  • Treat %Ash.ForbiddenField{} values as nil when dumping values for full_diff
  • Cover scalar and array cases in FullDiff.Helpers.dump_value/2
  • Add regression tests in AshPaperTrail.FullDiffTest
  • Document ForbiddenField->nil behavior for :full_diff in the Resource DSL docs
  • Note the fix under Unreleased in the changelog

Handle Ash.ForbiddenField safely in :full_diff and document behavior

Summary

This PR fixes GitHub issue 215 by making full-diff change tracking resilient to %Ash.ForbiddenField{} values introduced by Ash field policies. Previously, when a resource had field_policies and an attribute in changeset.data was replaced with %Ash.ForbiddenField{}, :full_diff would call Ash.Type.dump_to_embedded/3 with that value, receive :error, and crash with a MatchError. The new behavior treats any Ash.ForbiddenField encountered during full-diff dumping as nil, and documents this explicitly.


Changes

  • Full-diff value dumping

    • Updated dump_value/2 in helpers.ex to special-case Ash.ForbiddenField:
      • Added a clause:
        • dump_value(%Ash.ForbiddenField{}, _attribute), which now returns nil.
      • Updated the {:array, attr_type} case so that:
        • Each element is inspected; if it is %Ash.ForbiddenField{}, the dumped element becomes nil.
        • All non-forbidden elements still go through Ash.Type.dump_to_embedded/3 with the existing constraints.
      • Updated the generic scalar branch to guard against Ash.ForbiddenField and return nil in that case, instead of calling Ash.Type.dump_to_embedded/3.
    • This removes the {:ok, dumped_value} = :error crash path that issue 215 reported, while keeping the shape of the full-diff maps (from/to/unchanged) unchanged.
  • Tests

    • Extended the full-diff test suite in full_diff_test.exs with a new describe block that exercises Ash.ForbiddenField handling at the helper layer:
      • dump_value treats ForbiddenField in data as nil for scalar attributes:
        • Uses a simple string attribute metadata and calls Helpers.dump_value/2 with %Ash.ForbiddenField{field: :note, type: :attribute}.
        • Asserts the dumped value is nil.
      • dump_value treats ForbiddenField inside arrays as nil elements:
        • Uses an {:array, :string} attribute metadata.
        • Calls Helpers.dump_value/2 with ["visible", %Ash.ForbiddenField{}, "also visible"].
        • Asserts the dumped array is ["visible", nil, "also visible"].
    • These tests previously reproduced the same MatchError as in the issue; after the helper changes they now pass.
  • Documentation

    • Clarified the semantics of :full_diff in the generated DSL docs:
      • In the change_tracking_mode option docs in DSL-AshPaperTrail.Resource.md, added that when using :full_diff, any values hidden by field policies (i.e. %Ash.ForbiddenField{} in loaded data) are recorded as nil in the diff.
  • Changelog


Behavioral impact

  • For resources using change_tracking_mode: :full_diff:
    • If field_policies cause an attribute to be represented as %Ash.ForbiddenField{} in changeset.data, the full diff will now:
      • Treat that value as nil when computing the diff.
      • Avoid crashing.
      • Preserve the existing diff structure; for example, a previously forbidden-but-unchanged scalar will appear as %{unchanged: nil}, and a change from forbidden to a visible value will appear as %{from: nil, to: "new value"}.
    • For arrays, any element that is %Ash.ForbiddenField{} will be represented as nil in the dumped array before list-diffing.
  • No existing diff keys or envelope structure are changed, which minimizes compatibility risk for downstream consumers of version payloads.

Testing

  • Ran:
    • mix test test/ash_paper_trail/full_diff_test.exs
  • Result:
    • 42 tests, 0 failures, including the newly added ForbiddenField tests.

This PR thus:


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

…h-project#215)

- Treat %Ash.ForbiddenField{} values as nil when dumping values for full_diff
- Cover scalar and array cases in FullDiff.Helpers.dump_value/2
- Add regression tests in AshPaperTrail.FullDiffTest
- Document ForbiddenField->nil behavior for :full_diff in the Resource DSL docs
- Note the fix under Unreleased in the changelog
CHANGELOG.md Outdated

## Unreleased

### Bug Fixes:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the changelog is generated, no need to edit yourself.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0nce you remove this I'll merge.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I also removed a manual insert to the DSL docs. Didn't realize until later that they're also autogenerated

@zachdaniel
Copy link
Copy Markdown
Contributor

@M-Gonzalo this change looks good, but consider telling your agent that produces these descriptions to be way less verbose. This is just a few LoC, and that description is tons of prose 😓

@zachdaniel
Copy link
Copy Markdown
Contributor

Its still reasonable to document this behavior in some way, but yeah the DSL docs are autogenerated from the DSL istelf.

@zachdaniel
Copy link
Copy Markdown
Contributor

zachdaniel commented Dec 6, 2025

Looks like in trimming down the PR we removed the description change that documented this behavior. That would still be good to add 😄

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.

2 participants