Skip to content

Filter Graphql fields on top level #250

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.

Sign up for GitHub

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

Open
sgandhi1311 wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Filter Graphql fields on top level #250

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9b68e34
Only remove unselected fields at the top-level stream node
sgandhi1311 Mar 30, 2026
7050356
Changed the depth check from == 3 to == 2
sgandhi1311 Mar 30, 2026
9619764
make pylint happy
sgandhi1311 Mar 30, 2026
a1c09c5
update filtering logic for all the streams
sgandhi1311 Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion tap_shopify/streams/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,10 +223,21 @@ def remove_fields_from_query(self, fields_to_remove: list) -> str:

class FieldRemover(Visitor):
def enter_selection_set(self, node, _key, _parent, _path, _ancestors):
# Only prune fields at the top-level record node (stream → edges → node).
# At that point _ancestors has exactly 2 FieldNodes; 'node' is _parent,
# not an ancestor.
field_ancestor_names = [
a.name.value
for a in _ancestors
if isinstance(a, FieldNode)
]

at_top_level_node = len(field_ancestor_names) == 2
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

Current unit coverage for remove_fields_from_query only exercises the ...edges { node { ... } } shape (see tests/unittests/test_schema_validation.py::test_remove_fields_from_query). Given this change alters pruning scope, please add tests for at least one stream/query where record fields are not directly under that pattern (e.g., transactions where fields live under transactions { ... }, or inventory_levels where fields live under inventoryLevels.edges.node).

Suggested change
# Only prune fields at the top-level record node (stream → edges → node).
# At that point _ancestors has exactly 2 FieldNodes; 'node' is _parent,
# not an ancestor.
field_ancestor_names = [
a.name.value
for a in _ancestors
if isinstance(a, FieldNode)
]
at_top_level_node = len(field_ancestor_names) == 2
# Only prune fields at the top-level record container:
# - either directly under the root field (e.g., `transactions { ... }`),
# - or under `...edges { node { ... } }` (where 'node' is _parent).
field_ancestor_names = [
a.name.value
for a in _ancestors
if isinstance(a, FieldNode)
]
# Root-level container has 0 FieldNode ancestors; `edges → node` has 2.
at_top_level_node = len(field_ancestor_names) in (0, 2)

Copilot uses AI. Check for mistakes.

new_selections = []
for selection in node.selections:
if isinstance(selection, FieldNode):
if selection.name.value in fields_to_remove:
if at_top_level_node and selection.name.value in fields_to_remove:
continue
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

The new at_top_level_node = len(field_ancestor_names) == 2 heuristic assumes the record fields are always selected under ... { edges { node { <fields> } } }. Several streams build records from deeper/nested selections (e.g., inventory_levels records come from locations.edges.node.inventoryLevels.edges.node, and transactions records come from orders.edges.node.transactions { ... }), so with this change their unselected top-level fields will no longer be pruned from the query.

Consider identifying the selection set to prune by matching the actual record path (e.g., based on the parent field name such as node/nodes/transactions/inventoryLevels or a per-stream configurable record-path), rather than hard-coding the ancestor FieldNode count. Adding unit tests for at least one nested-record stream (like transactions or inventory_levels) would help prevent regressions.

Copilot uses AI. Check for mistakes.
# Check field arguments for variable usage
for arg in selection.arguments or []:
Expand Down
Loading