feat: add schema_paths for loading schema from local paths and installed packages AR-3#439
feat: add schema_paths for loading schema from local paths and installed packages AR-3#439Minister944 wants to merge 4 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…led packages Introduce a new `schema_paths` setting that builds the GraphQL schema from multiple sources. Each entry is resolved either as a dotted Python attribute path (e.g. `pkg.SCHEMA_DIR` or `pkg.get_schema_files`) — useful for pulling type definitions from installed packages — or as a local file/directory path. - schema.py: add `resolve_schema_paths` and `get_graphql_schema_from_paths` - main.py: wire `schema_paths` into both `client` and `graphqlschema` strategies - settings.py: add the `schema_paths` field and make the three schema sources (`schema_path`, `schema_paths`, `remote_schema_url`) mutually exclusive — providing more than one now raises InvalidConfiguration; simplify `using_remote_schema` accordingly - docs/README: document `schema_paths` and the mutual-exclusivity rule
cee0b53 to
79de200
Compare
|
|
||
| ### `schema_paths` entries | ||
|
|
||
| Each entry in `schema_paths` can be any of the following: |
There was a problem hiding this comment.
I would say it must, not "can".
| - **a path to a directory** - all `.graphql`, `.graphqls` and `.gql` files from it are included, eg. `./schemas/` | ||
| - **a path to a specific file** to be used, eg. `./foo/bar.graphql` | ||
|
|
||
| An import path that cannot be resolved (missing package, or the attribute no longer exists) raises a configuration error rather than being silently skipped. |
There was a problem hiding this comment.
Please remove it. You declared the lists of possible values, it's rather obvious it will raise an error. Moreover, we do not have similar mentions like "when providing remote_schema_url which is invalid URL, it will raise an error".
| def test_resolve_schema_paths_with_single_file(tmp_path, schema_str): | ||
| schema_file = tmp_path / "schema.graphql" | ||
| schema_file.write_text(schema_str, encoding="utf-8") | ||
|
|
||
| result = resolve_schema_paths([schema_file.as_posix()]) | ||
|
|
||
| assert result == [schema_file] | ||
|
|
||
|
|
||
| def test_resolve_schema_paths_with_directory(schemas_directory): | ||
| result = resolve_schema_paths([schemas_directory.as_posix()]) | ||
|
|
||
| names = {p.name for p in result} | ||
| assert "schema.graphql" in names | ||
| assert "user.graphql" in names | ||
|
|
||
|
|
||
| def test_resolve_schema_paths_with_multiple_local_sources( | ||
| tmp_path, schema_str, extra_type_str | ||
| ): | ||
| file1 = tmp_path / "schema.graphql" | ||
| file1.write_text(schema_str, encoding="utf-8") | ||
| file2 = tmp_path / "user.graphql" | ||
| file2.write_text(extra_type_str, encoding="utf-8") | ||
|
|
||
| result = resolve_schema_paths([file1.as_posix(), file2.as_posix()]) | ||
|
|
||
| assert file1 in result | ||
| assert file2 in result | ||
| assert len(result) == 2 |
There was a problem hiding this comment.
I don't understand what's the point to have three separate test cases here. The first one checks the single file, the second - a single directory, the last one: two files.
WDYT to have just a single test case, which tests reading local files, and provide as an input a list: single file and single directory.
| resolve_schema_paths(["some_pkg.SCHEMA_FILE"]) | ||
|
|
||
|
|
||
| def test_resolve_schema_paths_existing_file_skips_importlib( |
There was a problem hiding this comment.
Why do you propose to have a test like this? What's the purpose?
| mock_import.assert_not_called() | ||
|
|
||
|
|
||
| def test_resolve_schema_paths_accepts_file_with_any_extension(tmp_path, schema_str): |
There was a problem hiding this comment.
Again, why this test is needed?
| mock_import.assert_not_called() | ||
|
|
||
|
|
||
| def test_resolve_schema_paths_accepts_local_dir_with_dots_in_name( |
There was a problem hiding this comment.
I understand to show the code works for dots in filenames. But are you sure we absolutely need to have two different test cases for file and for directory?
| mock_import.assert_not_called() | ||
|
|
||
|
|
||
| def test_resolve_schema_paths_with_path_attribute_pointing_to_file( |
There was a problem hiding this comment.
Is this test case just a "file" version of the "directory" test test_resolve_schema_paths_with_path_attribute_pointing_to_directory? If so, why it lives here in the file, not directly below the other similar tests?
| assert result == [schema_file] | ||
|
|
||
|
|
||
| def test_resolve_schema_paths_raises_invalid_configuration_for_source_without_dot( |
There was a problem hiding this comment.
I would suggest to keep all the test cases "for this bad data, it raises InvalidConfiguration" in one place. We had some tests like this above, then other test cases, now we have again tests like this.
| return schema | ||
|
|
||
|
|
||
| def get_graphql_schema_from_path(schema_path: str) -> GraphQLSchema: |
There was a problem hiding this comment.
Did you consider just
def get_graphql_schema_from_path(schema_path: str) -> GraphQLSchema:
return get_graphql_schema_from_paths([Path(schema_path)])
With the current solution, bot "from_path" and "from_paths" uses similar blocks, but call them independently.
Introduce a new
schema_pathssetting that builds the GraphQL schema frommultiple sources. Each entry is resolved either as a dotted Python attribute
path (e.g.
pkg.SCHEMA_DIRorpkg.get_schema_files) - useful for pullingtype definitions from installed packages - or as a local file/directory path.
resolve_schema_pathsandget_graphql_schema_from_pathsschema_pathsinto bothclientandgraphqlschemastrategiesschema_pathsfield and make the three schema sources(
schema_path,schema_paths,remote_schema_url) mutually exclusive -providing more than one now raises InvalidConfiguration; simplify
using_remote_schemaaccordinglyschema_pathsand the mutual-exclusivity ruleCI/CD does not pass. This is an unrelated error that is being fixed as part of AR-6
Acknowledgements
While working on this, we noticed #431 by @esfomeado, which also tackled
loading the schema from more than one source. After discussing it as a team,
we went with a different approach - a dedicated
schema_pathssetting thatadditionally resolves dotted Python attribute paths from installed packages -
which we felt fit the project better. Thanks @esfomeado for raising the problem
and for your contribution! 🙏
Co-authored-by: esfomeado 1906254+esfomeado@users.noreply.github.com