Skip to content
This repository was archived by the owner on Feb 6, 2025. It is now read-only.

Add a function that computes the fingerprint of a schema #737

Merged
merged 17 commits into from
Jan 27, 2020

Conversation

pmantica1
Copy link
Contributor

I'll wait until we move to ReadTheDocs to add documentation for this function.

@codecov
Copy link

codecov bot commented Jan 22, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@47a7655). Click here to learn what that means.
The diff coverage is 83.06%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #737   +/-   ##
=========================================
  Coverage          ?   94.35%           
=========================================
  Files             ?      105           
  Lines             ?     7865           
  Branches          ?        0           
=========================================
  Hits              ?     7421           
  Misses            ?      444           
  Partials          ?        0
Impacted Files Coverage Δ
...l_compiler/query_pagination/pagination_planning.py 86.66% <ø> (ø)
graphql_compiler/api/redisgraph.py 0% <0%> (ø)
graphql_compiler/api/sql/mssql.py 0% <0%> (ø)
graphql_compiler/api/sql/mysql.py 0% <0%> (ø)
graphql_compiler/api/sql/__init__.py 0% <0%> (ø)
graphql_compiler/api/sql/postgres.py 0% <0%> (ø)
graphql_compiler/api/orientdb.py 0% <0%> (ø)
graphql_compiler/schema/schema_info.py 86.66% <80.76%> (ø)
...l_compiler/query_pagination/parameter_generator.py 96.87% <96.82%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47a7655...424aa01. Read the comment docs.

Copy link
Contributor

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

Looking at the implementation here, I'm a bit worried that the order of fields in schema directives is sorted.

Could you add some test cases that include directives that take multiple arguments, that are used e.g. FIELD_DEFINITION locations, and see if the order of arguments to such directives is also sorted? The input would look something like:

directive @custom_directive(b: String, a: Int) on FIELD_DEFINITION

type Foo {
    my_field @custom_directive(b: "abc", a: 123)
}

and the output should have the directive argument order sorted in both the directive definition and its use in the schema.

If the lexicographic_sort_schema function doesn't already do this, we should open an issue. I think we should also offer to make a PR for it, since the GraphQL core library already does most of the heavy lifting and this should not be difficult for us to add.

I have a couple of smaller nits to pick at (things like typos in docstrings etc.) but figured I'd start with the big stuff.

@pmantica1
Copy link
Contributor Author

pmantica1 commented Jan 23, 2020

Looking at the implementation here, I'm a bit worried that the order of fields in schema directives is sorted.

Could you add some test cases that include directives that take multiple arguments, that are used e.g. FIELD_DEFINITION locations, and see if the order of arguments to such directives is also sorted? The input would look something like:

directive @custom_directive(b: String, a: Int) on FIELD_DEFINITION

type Foo {
    my_field @custom_directive(b: "abc", a: 123)
}

and the output should have the directive argument order sorted in both the directive definition and its use in the schema.

If the lexicographic_sort_schema function doesn't already do this, we should open an issue. I think we should also offer to make a PR for it, since the GraphQL core library already does most of the heavy lifting and this should not be difficult for us to add.

I have a couple of smaller nits to pick at (things like typos in docstrings etc.) but figured I'd start with the big stuff.

I think that test_directive_defintion_argument_order and test_directive_definition_location_order are already testing in part what you are referring to. In my latest commit I added test_argument_order_of_field_definition_directive and premptively fixed some typos.

Copy link
Contributor

@obi1kenobi obi1kenobi left a comment

Choose a reason for hiding this comment

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

My remaining nits. I'd still love to get the "repeated directive on the same field" test case in there as well.

Comment on lines 10 to 12
def _compare_schema_fingerprints(
test_case: unittest.TestCase, schema_text1: str, schema_text2: str, expect_equality: bool = True
) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't say I'm a fan of the name of this function or of expect_equality = True as an approach. A lot of the tests end in _compare_schema_fingerprints(self, schema1, schema2) which does not look like an assertion, and even if you assume it is one, it's not clear whether it's comparing for equality or inequality.

I'd suggest either having different functions for equality and inequality, e.g. assert_equal_fingerprints() and assert_not_equal_fingerprints(). If you are worried about the repetition between them, you could also pull compute_schema_fingerprint(build_ast_schema(parse(x))) into a function of its own.

Comment on lines 167 to 168
reason="Type extension information is lost when a schema is print and parsed. "
"See https://github.com/graphql/graphql-js/issues/2386",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think it might be a bit more readable if you surround the string literal here in parens: reason=(...)

pmantica1 and others added 3 commits January 24, 2020 15:17
Co-Authored-By: Predrag Gruevski <[email protected]>
@pmantica1
Copy link
Contributor Author

My remaining nits. I'd still love to get the "repeated directive on the same field" test case in there as well.

Added test_reordered_repeatable_directives_at_field_definition. I missed your latest commits. I will address them in an upcoming commit.

is an *explicit non-goal* here, changing a schema's fingerprint will not be considered
a breaking change.

The fingerprint may not include information about directives at field definitions.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is way too specific. There are probably half a dozen other kinds of things that are also not covered (e.g. directives on types, on scalars, on extensions) and yet are not mentioned. Let's not actively mislead people by listing only one of the many caveats present here.

I'd keep it more vague and say that this fingerprint is done on a best-effort basis and has some known issues at the moment, and direct people to the discussion in this PR for more details.

@obi1kenobi obi1kenobi merged commit 4a0fe10 into master Jan 27, 2020
@obi1kenobi obi1kenobi deleted the add-hash-function branch January 27, 2020 19:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants