search

torchbox / wagtail-grapple

A Wagtail app that makes building GraphQL endpoints a breeze!
https://wagtail-grapple.readthedocs.io/en/latest/
Other
151 stars 57 forks source link

Uniform ID Type for pages and page Queries #350

Closed estyxx closed 10 months ago

estyxx commented 11 months ago

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.

zerolab commented 11 months ago

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

estyxx commented 11 months ago

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

jams2 commented 10 months ago

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 commented 10 months ago

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

estyxx commented 10 months ago

@zerolab I rebased this PR :)