Skip to content

fix: enforce NonNegativeInt on GraphQL pagination arguments#9965

Open
iddocohen wants to merge 4 commits into
developfrom
fix/nonnegativeint-pagination-validation
Open

fix: enforce NonNegativeInt on GraphQL pagination arguments#9965
iddocohen wants to merge 4 commits into
developfrom
fix/nonnegativeint-pagination-validation

Conversation

@iddocohen

@iddocohen iddocohen commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a NonNegativeInt GraphQL scalar and applies it to the pagination arguments (limit, offset, log_limit, log_offset) of the specialized top-level queries: branch, event, task, search, relationship, account-token, resource-manager and diff-tree. Negative or non-integer pagination values on these queries are now rejected at the schema level with an error that names the offending argument and its location.

The scalar lives in backend/infrahub/graphql/scalars.py (next to FixedGenericScalar).

This supersedes #7610 (originally opened from a personal fork before I joined OpsMill), rebased onto current develop and reworked per the review there.

Context / scope

#7474 (late errors for negative offset/limit) was already fixed and released in v1.9.5 by #9203, which added a resolver-level validate_offset_and_limit() guard in default_paginated_list_resolver — the resolver used by the generic node list queries. That guard covers generic node pagination but not the specialized query resolvers.

This PR closes that gap by validating declaratively at the schema level on exactly those specialized queries. The generic node filters are intentionally left as Int — they are already validated by #9203, and switching them to NonNegativeInt would be a breaking change for every client (frontend, SDK, internal node-id fetch queries) that paginates a node with an Int variable.

Review feedback addressed (from #7610)

  • @ogenstad: raise ValidationError rather than GraphQLError so the response keeps the argument type name and source location. The scalar raises infrahub.exceptions.ValidationError, which graphql-core wraps as Expected value of type 'NonNegativeInt', found -1; Value must be a non-negative integer with locations populated.
  • Docstrings + type hints on the scalar; ValueError/TypeError handled.

Notable fixes beyond the original PR

  • parse_literal accepts the variables argument the executor passes, so a paginated query that also declares query variables is coerced correctly instead of failing argument validation (the original raised a spurious invalid value).
  • parse_value validates strictly: booleans, strings and fractional floats are rejected (whole-number floats accepted, matching the built-in Int), so variable inputs enforce the same integer contract as inline literals.

Changes

  • New NonNegativeInt in backend/infrahub/graphql/scalars.py, with unit tests in backend/tests/unit/graphql/types/test_scalars.py.
  • Int()NonNegativeInt() on the pagination args of the eight specialized query modules.
  • Regenerated schema/schema.graphql (scalar + the eight fields) and the frontend generated GraphQL files; updated the frontend query documents for the specialized queries and mapped NonNegativeIntnumber in gql.tada / graphql-codegen.
  • Component tests for the specialized queries; the branch resolver's now-redundant negative-offset check was removed (the scalar guarantees it).

Test plan

  • uv run invoke format / lint, mypy — clean.
  • Backend unit + affected component tests pass locally against a Neo4j testcontainer; CI green.
  • Frontend codegen + betterer/biome/knip all pass.

Closes #7610.

Add a NonNegativeInt GraphQL scalar and apply it to the limit, offset,
log_limit and log_offset arguments across the branch, event, task,
search, relationship, account-token, resource-manager and diff-tree
queries, as well as the generated node filters. Negative or non-integer
pagination values are now rejected at the schema level with an error that
names the offending argument and its source location.

This extends the resolver-level guard on the main node resolver to every
paginated query by validating declaratively at the schema level. The
scalar raises ValidationError so the executor wraps it with the argument
type name and location, matching the behaviour of the built-in scalars.

parse_literal accepts the variables argument passed by the executor, so a
paginated query that also declares query variables is coerced correctly
instead of failing argument validation.

Update the frontend query documents and regenerate the GraphQL schema and
frontend generated types accordingly.
@iddocohen
iddocohen requested review from a team as code owners July 19, 2026 08:22
@github-actions github-actions Bot added group/backend Issue related to the backend (API Server, Git Agent) group/frontend Issue related to the frontend (React) labels Jul 19, 2026
@iddocohen
iddocohen requested a review from ogenstad July 19, 2026 08:22

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2 issues found across 30 files

Confidence score: 4/5

  • In backend/infrahub/graphql/queries/branch.py, InfrahubBranchQueryList.limit accepts NonNegativeInt while infrahub_branch_resolver rejects values < 1, so a client can pass schema validation with limit=0 and then fail at runtime with a different error path; align the schema and resolver rule (or normalize 0 in the resolver) before merging to avoid confusing client-side failures.
  • In backend/infrahub/graphql/queries/account.py, the GraphQL type change appears out of sync with the linked SDK fixture (opsmill/infrahub-sdk-python/tests/fixtures/unit/test_graphql_plugin/schema.graphql), which can cause downstream test or codegen drift even if this repo is otherwise stable; update the fixture (or coordinate a companion SDK PR) to de-risk integration mismatches.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="backend/infrahub/graphql/queries/account.py">

<violation number="1" location="backend/infrahub/graphql/queries/account.py:67">
P3: The linked SDK repo's test fixture schema at opsmill/infrahub-sdk-python/tests/fixtures/unit/test_graphql_plugin/schema.graphql:8577 still declares `InfrahubAccountToken(limit: Int, offset: Int)` rather than `NonNegativeInt`, which will be out of date once this PR merges. If the SDK loads this fixture for GraphQL plugin tests or codegen, those tests could fail or produce stale generated code.</violation>
</file>

<file name="backend/infrahub/graphql/queries/branch.py">

<violation number="1" location="backend/infrahub/graphql/queries/branch.py:155">
P3: InfrahubBranchQueryList's limit arg uses NonNegativeInt (allows 0), but infrahub_branch_resolver rejects limit < 1. Clients passing limit=0 get a schema-level pass then a ValidationError in the resolver with a different phrasing. Consider either documenting the constraint or aligning the resolver check with NonNegativeInt's semantics.</violation>
</file>

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread backend/infrahub/graphql/types/scalars.py Outdated
Comment thread backend/infrahub/graphql/queries/account.py
Comment thread backend/infrahub/graphql/queries/branch.py
…odegen

The NonNegativeInt scalar defaulted to `unknown` in the generated GraphQL
types, so query documents using it for limit/offset inferred `unknown`
variables and failed TypeScript checks where a number was expected. Map
NonNegativeInt to number in the gql.tada setupSchema augmentation and in
the graphql-codegen scalar config, and regenerate the type caches.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

0 issues found across 4 files (changes from recent commits).

Shadow auto-approve: would not auto-approve. Auto-approval blocked by 4 unresolved issues from previous reviews.

Re-trigger cubic

@codspeed-hq

codspeed-hq Bot commented Jul 19, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 12 untouched benchmarks


Comparing fix/nonnegativeint-pagination-validation (c00845a) with develop (2f09386)

Open in CodSpeed

Reject booleans, strings and fractional floats in the NonNegativeInt
variable path instead of coercing them, so variable inputs enforce the
same integer contract as inline integer literals (whole-number floats are
still accepted, matching the built-in Int scalar).

Drop the now-unreachable negative-offset check in the branch resolver; the
NonNegativeInt scalar rejects negative offsets before the resolver runs.
@iddocohen

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed below.

1. scalars.py — strict integer validation (P2): fixed.
The literal path already rejected non-integers (only IntValueNode is accepted), but the variable path went through int(value), which coerced 1.9, true and "1". _validate now accepts only genuine integers (and whole-number floats, matching graphql-core's built-in Int), rejecting booleans, strings and fractional floats — so variable and literal inputs enforce the same integer contract. Added test_builtin_tag_rejects_fractional_limit_variable and test_builtin_tag_rejects_boolean_limit_variable.

2. branch.pylimit=0 vs resolver rule (P3): partially addressed.
The offset < 0 resolver check was dead code once offset became NonNegativeInt (the scalar rejects negatives before the resolver runs), so I removed it. The limit < 1 check is intentional, tested branch semantics — a branch listing with limit=0 is meaningless — so it stays. limit=0 therefore passes the scalar (0 is non-negative) and is rejected by the resolver with the more specific "limit must be >= 1", which is the correct message for that stricter, branch-only constraint.

3. Frontend scalar mapping (P2): already fixed.
NonNegativeInt: "number" was added to graphql.config.ts and to the gql.tada setupSchema augmentation (tada-scalars.d.ts) in commit caeed04, with the type caches regenerated. Your follow-up review on that commit reported 0 issues.

4. SDK fixture schema.graphql:8577 (P3): out of scope.
That fixture lives in the separate opsmill/infrahub-sdk-python repo and is a static copy used by the SDK's own tests; it doesn't affect this PR's schema or CI. Keeping the SDK fixture in sync is a follow-up in that repo, not part of this change.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

0 issues found across 3 files (changes from recent commits).

Shadow auto-approve: would not auto-approve. Auto-approval blocked by 3 unresolved issues from previous reviews.

Re-trigger cubic

Revert the generic node filters in the GraphQL schema manager back to Int.
Generic node list pagination is already validated at the resolver level, and
switching those arguments to NonNegativeInt was a breaking change for every
client that paginates nodes with an Int variable (including the internal
node-id fetch queries used by computed attributes, display labels and HFIDs).

NonNegativeInt now applies only to the branch, event, task, search,
relationship, account-token, resource-manager and diff-tree queries, which the
resolver-level guard does not cover. Move the scalar next to the existing
FixedGenericScalar and cover it with unit tests. Revert the frontend documents
and tests that paginate generic nodes back to Int and regenerate the schema and
frontend types.
@iddocohen

Copy link
Copy Markdown
Contributor Author

Scoped the change down after the backend-tests-functional failure. Applying NonNegativeInt to the generic node filters in the schema manager was a breaking change for every client paginating nodes with an Int variable — it broke the internal node-id fetch queries used by computed attributes / display labels / HFIDs. Generic node pagination is already validated at the resolver level (#9203), so I reverted the generic filters back to Int. NonNegativeInt now applies only to the specialized queries the resolver guard doesn't cover (branch, event, task, search, relationship, account-token, resource-manager, diff-tree). This shrinks the schema diff to just the scalar + those 8 fields and reverts the frontend/tests that paginate generic nodes back to Int. The scalar also moved next to FixedGenericScalar in graphql/scalars.py with unit tests.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 22 files (changes from recent commits).

Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread backend/infrahub/graphql/queries/search.py
@iddocohen

Copy link
Copy Markdown
Contributor Author

Latest cubic review:

1 (P2, scalars path in description): Fixed — updated the PR description; the scalar lives in backend/infrahub/graphql/scalars.py.

2 (P1, negative pagination on relationship/hierarchy/IPAM): This is pre-existing behavior, not a regression from this PR. Those nested/hierarchy/IPAM pagination args were Int (unvalidated) on develop, and #9203's guard only covers the generic node list resolver (default_paginated_list_resolver), not those paths. The broad version of this PR briefly changed them via the schema manager, but that was a breaking API change (it broke the internal node-id fetch queries and would break any SDK/frontend client paginating with an Int variable), so it was reverted to match the agreed scope: the 8 specialized top-level queries #9203 doesn't cover. Adding non-negative validation to nested/hierarchy/IPAM pagination is a separate change with the same breaking/back-compat trade-off, and I've left it out of this PR deliberately. Happy to file a follow-up for it if we want to tackle it (schema-level vs per-resolver) as its own decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

group/backend Issue related to the backend (API Server, Git Agent) group/frontend Issue related to the frontend (React)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant