Fix relationship issue 28#419
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bdddb6e6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lib/ash_json_api/serializer.ex
Outdated
| case Map.fetch(identifier_meta, AshJsonApi.Resource.encode_primary_key(record)) do | ||
| {:ok, meta_for_identifier} when is_map(meta_for_identifier) and | ||
| map_size(meta_for_identifier) > 0 -> | ||
| Map.put(payload, :meta, meta_for_identifier) |
There was a problem hiding this comment.
serialize_to_many_relationship/5 now unconditionally overlays request.assigns.relationship_identifier_meta onto each relationship identifier, but that assign is populated from raw request meta in add_to_relationship/2 without checking whether the relationship has a relationship_meta mapping (or whether keys are mapped). As a result, POST /relationships can return arbitrary meta keys that were never persisted (e.g., on unmapped relationships or unmapped keys), so the write response can disagree with stored data and later reads.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a bit of a complex one, but I think the robot is right here. We didn't necessarily write all of those fields, or they could have been written differently by the action. So we actually should extract them from the related value (i.e reverse the mapping and get them from the result).
With that said, even that isn't perfect because not all inputs have corresponding outputs. We may need two configs here. One for relationship meta in, and one for relationship meta out.
There was a problem hiding this comment.
Sounds good, here is what I am planning on doing. I'll remove the request-meta echoing and ensure the relationship identifier meta is derived from the resulting related/join data path instead of raw input, so responses reflect persisted behavior rather than what was merely submitted. I'll keep a single relationship_meta mapping for now rather than splitting into separate in/out configs to keep this fix focused, reduce DSL/API surface area, and avoid introducing additional configuration complexity. Does that sound good or do you want the two configs right now?
There was a problem hiding this comment.
I would prefer two configs given that in Ash we don't assume that any input maps directly to output.
There was a problem hiding this comment.
Ok, I’ve updated this to support separate mappings for relationship identifier meta input and output. Instead of only relationship_meta, resources can now configure relationship_meta_in (used when parsing incoming relationship identifier meta into join attributes for manage_relationship) and relationship_meta_out (used when serializing relationship identifier meta from resulting join data). relationship_meta remains as a backward-compatible fallback when split mappings aren’t provided. In AshJsonApi.Controllers.Helpers, relationship writes now use the _in mapping and then load the updated record’s join relationship so serializer has persisted result data available. In AshJsonApi.Serializer, relationship identifier meta uses the _out mapping and is derived from loaded join rows instead of echoing request input. I also updated the acceptance test to use different in/out keys (note in, note_out out) and assert that output follows the out mapping while excluding unmapped request keys.
There was a problem hiding this comment.
relationship_meta remains as a backward-compatible fallback when split mappings aren’t provided.
I would prefer not to have this. Its not a reliable thing to use and would be a footgun for users to make mistakes.
There was a problem hiding this comment.
Sounds good, I will remove that. Is there anything else that you would like removed or changed before I update the pull request?
There was a problem hiding this comment.
Nope! I think we'll be good to go after that 😄
There was a problem hiding this comment.
Perfect, I removed the single flow so now the only options are the input and output!
lincolnhuls
force-pushed
the
fix-json-issue28
branch
from
770e25b to
66c5900
Compare
|
Okay last thing is now that there are conflicts in the serializer, sorry! 🙈 . Once thats done we can merge 😄 |
lincolnhuls
force-pushed
the
fix-json-issue28
branch
from
9354724 to
ec0e858
Compare
|
Ok, I think I cleared up that issue! |
2 participants
Contributor checklist
Leave anything that you believe does not apply unchecked.
I’ve added support for join-table fields via JSON:API meta on relationship resource identifiers. Resources can now declare relationship_meta in their json_api config, which maps per-relationship meta keys to join attributes. In AshJsonApi.Controllers.Helpers, many-to-many relationship routes use that mapping to turn incoming identifiers with meta into the single map of fields that manage_relationship expects (including both the related ID and the derived join attributes), and we retain the meta per identifier so the response can echo it back. In AshJsonApi.Serializer, relationship responses now attach that meta onto each identifier, and there’s a ManyToMany-specific add_relationship_meta/4 clause wired to the same mapping when join rows are loaded. I extended the existing relationship acceptance test to assert that posting identifiers with meta results in the same meta being present on the identifiers in the response, while behavior for relationships without relationship_meta or without meta remains unchanged.