Skip to content

Conversation

StevenACoffman
Copy link
Collaborator

@StevenACoffman StevenACoffman commented Jun 28, 2025

Follow-up to #379 and #380 and #381

Recently, there was the beginning of an effort by @tomoikey to discourage the use of global variables in gqlparser.

Those changes, per my comment here, we have inadvertently made the order in which validation rules are applied non-deterministic.

This can cause problems in downstream projects in their continuous integration tests, as they might not get perfectly identical results due to ordering differences. This PR attempts to make the ordering of validation rules deterministic.

Note: I do NOT think this is now the best order of validation rule precedence, NOR do I think the original order was that either. They were and still are sorted arbitrarily only to have a deterministic precedence.

Also, if you were to add custom rule(s) using BOTH the deprecated and non-deprecated methods, and then to compare the validation order precedence, they would not be perfectly identical, because this is not expected to be of any consequence.

Signed-off-by: Steve Coffman steve@khanacademy.org

@coveralls
Copy link

coveralls commented Jun 28, 2025

Coverage Status

coverage: 87.174% (-0.2%) from 87.336%
when pulling db264eb on deterministic_validation_order
into 46a464d on master.

@StevenACoffman StevenACoffman force-pushed the deterministic_validation_order branch from f98809e to 88e89b4 Compare June 28, 2025 22:08
Signed-off-by: Steve Coffman <steve@khanacademy.org>
@StevenACoffman StevenACoffman force-pushed the deterministic_validation_order branch from 88e89b4 to db264eb Compare June 28, 2025 22:21
@StevenACoffman StevenACoffman changed the title Ensure Validation order is deterministic Ensure Validation Rule order is deterministic Jun 28, 2025
@StevenACoffman StevenACoffman merged commit f638740 into master Jun 28, 2025
7 checks passed
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