Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add Relay ID translation in mutation and queries #109

Merged
merged 2 commits into from
Feb 6, 2024

Conversation

rbino
Copy link
Contributor

@rbino rbino commented Jan 29, 2024

Adds a new option for queries and mutations that defines which arguments or attributes will use a global Relay ID and their type. This allows automatically decoding them before hitting their action.

Contributor checklist

  • Features include unit/acceptance tests

@rbino rbino marked this pull request as draft January 29, 2024 23:41
@rbino
Copy link
Contributor Author

rbino commented Jan 29, 2024

@zachdaniel the feature is working, but it's missing some error handling and some more niceties. Opening as draft to collect feedback on the direction/naming/etc.

@rbino rbino force-pushed the relay-id-translator branch 2 times, most recently from 46c2627 to b4e8bcc Compare January 29, 2024 23:46
@zachdaniel
Copy link
Contributor

I'm wondering if there is a way for us to figure this out automatically for cases with manage_relationship?

@rbino
Copy link
Contributor Author

rbino commented Jan 30, 2024

I think this would be orthogonal to managed_relationships, from what I understand, the main purpose of managed_relationships is generating input types from nested relationships, so I think it's mainly used for nested resource creation (e.g. create_post_with_tags) but less so with relationships that don't create the related resource (e.g. create_post_with_author, which doesn't create the author). Also, an ID could also be passed in a non-standard argument (like a map) and handled with a manual change, which wouldn't be detectable.

The high level idea I have in mind to see if this can be automatically generated is more or less this:

  • Do something similar to managed_relationship and retrieve all changes in the action which use a ManageRelationship change
  • From these changes, filter the ones which map the argument to a primary key of the related resource
  • Emit the translation middleware for those arguments (also taking care of the nesting, e.g. the wrapping input object)

Given we would still need the manual way to do this to provide an escape hatch for cases not covered by the autogeneration, I don't know if you prefer fixing the manual way and then working on the autogeneration in a separate PR or start directly with the autogeneration.

@zachdaniel
Copy link
Contributor

Yes, great points. We need both 👍 Doesn't really matter which one we start with 😄

@zachdaniel
Copy link
Contributor

As for managed relationship type generation, you should be able to tap into our existing logic for generating types for managed relationships

@rbino rbino force-pushed the relay-id-translator branch 2 times, most recently from 9197459 to d676af3 Compare January 31, 2024 16:33
@rbino rbino marked this pull request as ready for review January 31, 2024 16:33
@rbino
Copy link
Contributor Author

rbino commented Jan 31, 2024

@zachdaniel ready for review, this implements the manual part as discussed above. I will then open an issue to track the implementation of the automatically derived translations.

Adds a new option for queries and mutations that defines which arguments or
attributes will use a global Relay ID and their type. This allows automatically
decoding them before hitting their action.

This paves the way to automatic translation derived from the arguments, which
will be implemented subsequently.
@rbino
Copy link
Contributor Author

rbino commented Feb 6, 2024

@zachdaniel rebased to fix the conflicts in the cheatsheet. I've also shortened the docs in the DSL and linked to the Relay guide instead as suggested by the Spark warning.

@zachdaniel zachdaniel merged commit 66d2f44 into ash-project:main Feb 6, 2024
13 checks passed
@zachdaniel
Copy link
Contributor

🚀 Thank you for your contribution! 🚀

rbino added a commit to rbino/edgehog that referenced this pull request Feb 6, 2024
Bring in the global ID translations merged in
ash-project/ash_graphql#109
rbino added a commit to rbino/edgehog that referenced this pull request Feb 8, 2024
Bring in the global ID translations merged in
ash-project/ash_graphql#109

Signed-off-by: Riccardo Binetti <[email protected]>
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