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

Uniform ID Type for pages and page Queries #350

Merged
merged 2 commits into from
Aug 30, 2023

Conversation

estyxx
Copy link
Contributor

@estyxx estyxx commented Aug 2, 2023

Currently, in the Wagtail GraphQL API of wagtail-grapple, there is an inconsistency in the input types for the pages and page queries. While the pages query expects an ID as an input, the page query expects an Int. This has been leading to unnecessary conversion between string and integer.

Proposed Changes

I propose to make the input types consistent by changing the id in the page query to be of type ID (the same as in the pages query).

The change can be made in the following file:
grapple/types/pages.py#L336

By changing the return type to graphene.ID, it ensures uniformity across the queries and eradicates the need for conversions between string and integer.

I updated the documentation as well.

@estyxx estyxx force-pushed the uniform-pages-query-types branch from 9743490 to 28050ed Compare August 2, 2023 16:26
@estyxx estyxx force-pushed the uniform-pages-query-types branch from bb34d17 to 4b259f9 Compare August 2, 2023 16:56
@zerolab
Copy link
Member

zerolab commented Aug 2, 2023

@estyxx the pre-commit failure is from the recent ruff update. can you update tests/settings.py:L190 to be # noqa: S105?

@estyxx
Copy link
Contributor Author

estyxx commented Aug 2, 2023

~~ Mmm it's already commented with #noqa S105 on my branch 🤔 ~~
Ah was incorrect! fixed!

@jams2
Copy link
Contributor

jams2 commented Aug 11, 2023

Forgive me if this is an ignorant question, but why should we use ID rather than Int? I assume that the id field maps to Page.id, which is an integer column.

@zerolab
Copy link
Member

zerolab commented Aug 11, 2023

The benefit of using ID over Int everywhere is that it can support models (e.g. snippets) with non-int primary keys. Also https://graphql.com/learn/scalars-objects-lists/#the-id-type mentions lots of frameworks/tools have better support for ID out of the box.

Now, this PR only changes it for pages, so if we commit, we should go all the way.

The reason I haven't merged this yet is that I am still deciding as it is a potentially breaking change

Copy link
Member

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

@estyxx having "slept on it", it is a worthy change.
Any chance you could rebase?

@estyxx estyxx force-pushed the uniform-pages-query-types branch from 87f2a58 to 0b72e6e Compare August 30, 2023 09:02
@estyxx
Copy link
Contributor Author

estyxx commented Aug 30, 2023

@zerolab I rebased this PR :)

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.

3 participants