Add id field to graphql entities #2045
Conversation
Summary by CodeRabbit
WalkthroughChanged multiple GraphQL node classes to inherit from strawberry.relay.Node (Relay-compatible), which caused tests to expect an additional "_id" GraphQL field on those node types. No resolver logic or other fields were added or modified. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
backend/apps/github/api/internal/nodes/repository.py (1)
43-46: Optional: centralize id resolvers or adopt Relay Node for consistencyYou’ve repeated identical id resolvers across many classes. To reduce duplication and standardize behavior:
- Option A (lightweight): Introduce mixins, e.g., GitHubNodeIdMixin (returns self.node_id) and KeyIdMixin (returns self.key), and inherit in each Node.
- Option B (more structured): Adopt strawberry.relay.Node across types for a canonical global id pattern.
Happy to draft a small mixin and an example application if you want to consolidate in this PR or a follow-up.
backend/apps/owasp/api/internal/nodes/post.py (1)
22-25: Prefer using pk and return a string for ID stabilityTo avoid any confusion with method/attribute shadowing and to ensure a consistent string ID across clients, consider returning the model PK as a string.
@strawberry.field def id(self) -> strawberry.ID: """Resolve a unique identifier.""" - return self.id + return str(self.pk)backend/tests/apps/github/api/internal/nodes/organization_test.py (2)
31-31: Make the field set assertion less brittle (allow future fields)Exact set equality makes this test break whenever a legitimate field is added. Recommend asserting a superset instead.
Apply this diff:
- assert field_names == expected_field_names + # Ensure required fields are present; allow additional fields as schema evolves + assert expected_field_names.issubset(field_names)
14-31: Add a targeted type assertion for the id field (prefer GraphQL ID scalar)To guard contract, add a small test that asserts the "id" field exists and has the expected scalar type. Strawberry may surface ID as strawberry.ID or str depending on usage, so accept both, prioritizing strawberry.ID if present.
You can append the following test near the others:
# Add at the top with imports: import strawberry # if not already available # Add alongside other tests in TestOrganizationNode: def test_resolve_id_type(self): id_field = next( (f for f in OrganizationNode.__strawberry_definition__.fields if f.name == "id"), None, ) assert id_field is not None # Depending on Strawberry internals, ID may appear as strawberry.ID or str. assert id_field.type in (strawberry.ID, str)Optionally, if you have access to the built GraphQL schema in tests, it’s even better to introspect the schema and assert Organization.id is ID! directly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
backend/apps/github/api/internal/nodes/issue.py(1 hunks)backend/apps/github/api/internal/nodes/milestone.py(1 hunks)backend/apps/github/api/internal/nodes/organization.py(1 hunks)backend/apps/github/api/internal/nodes/pull_request.py(1 hunks)backend/apps/github/api/internal/nodes/release.py(1 hunks)backend/apps/github/api/internal/nodes/repository.py(1 hunks)backend/apps/owasp/api/internal/nodes/common.py(1 hunks)backend/apps/owasp/api/internal/nodes/event.py(2 hunks)backend/apps/owasp/api/internal/nodes/post.py(2 hunks)backend/apps/owasp/api/internal/nodes/project_health_metrics.py(1 hunks)backend/apps/owasp/api/internal/nodes/sponsor.py(2 hunks)backend/tests/apps/github/api/internal/nodes/issue_test.py(1 hunks)backend/tests/apps/github/api/internal/nodes/milestone_test.py(1 hunks)backend/tests/apps/github/api/internal/nodes/organization_test.py(1 hunks)backend/tests/apps/github/api/internal/nodes/release_test.py(1 hunks)backend/tests/apps/github/api/internal/nodes/repository_test.py(1 hunks)backend/tests/apps/owasp/api/internal/nodes/project_health_metrics_test.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.
Applied to files:
backend/apps/owasp/api/internal/nodes/sponsor.pybackend/apps/owasp/api/internal/nodes/event.pybackend/apps/owasp/api/internal/nodes/post.py
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
Applied to files:
backend/apps/owasp/api/internal/nodes/sponsor.pybackend/apps/owasp/api/internal/nodes/event.pybackend/apps/owasp/api/internal/nodes/post.py
🧬 Code Graph Analysis (9)
backend/apps/github/api/internal/nodes/milestone.py (10)
backend/apps/github/api/internal/nodes/pull_request.py (1)
id(26-28)backend/apps/github/api/internal/nodes/issue.py (1)
id(28-30)backend/apps/owasp/api/internal/nodes/common.py (1)
id(13-15)backend/apps/github/api/internal/nodes/release.py (1)
id(29-31)backend/apps/owasp/api/internal/nodes/event.py (1)
id(27-29)backend/apps/github/api/internal/nodes/repository.py (1)
id(44-46)backend/apps/github/api/internal/nodes/organization.py (1)
id(43-45)backend/apps/owasp/api/internal/nodes/post.py (1)
id(23-25)backend/apps/owasp/api/internal/nodes/project_health_metrics.py (1)
id(50-52)backend/apps/owasp/api/internal/nodes/sponsor.py (1)
id(22-24)
backend/apps/github/api/internal/nodes/repository.py (5)
backend/apps/github/api/internal/nodes/milestone.py (1)
id(31-33)backend/apps/github/api/internal/nodes/pull_request.py (1)
id(26-28)backend/apps/github/api/internal/nodes/issue.py (1)
id(28-30)backend/apps/github/api/internal/nodes/release.py (1)
id(29-31)backend/apps/github/api/internal/nodes/organization.py (1)
id(43-45)
backend/apps/github/api/internal/nodes/release.py (5)
backend/apps/github/api/internal/nodes/milestone.py (1)
id(31-33)backend/apps/github/api/internal/nodes/pull_request.py (1)
id(26-28)backend/apps/github/api/internal/nodes/issue.py (1)
id(28-30)backend/apps/github/api/internal/nodes/repository.py (1)
id(44-46)backend/apps/github/api/internal/nodes/organization.py (1)
id(43-45)
backend/apps/owasp/api/internal/nodes/common.py (10)
backend/apps/github/api/internal/nodes/milestone.py (1)
id(31-33)backend/apps/github/api/internal/nodes/pull_request.py (1)
id(26-28)backend/apps/github/api/internal/nodes/issue.py (1)
id(28-30)backend/apps/github/api/internal/nodes/release.py (1)
id(29-31)backend/apps/owasp/api/internal/nodes/event.py (1)
id(27-29)backend/apps/github/api/internal/nodes/repository.py (1)
id(44-46)backend/apps/github/api/internal/nodes/organization.py (1)
id(43-45)backend/apps/owasp/api/internal/nodes/post.py (1)
id(23-25)backend/apps/owasp/api/internal/nodes/project_health_metrics.py (1)
id(50-52)backend/apps/owasp/api/internal/nodes/sponsor.py (1)
id(22-24)
backend/apps/owasp/api/internal/nodes/sponsor.py (3)
backend/apps/owasp/api/internal/nodes/common.py (1)
id(13-15)backend/apps/owasp/api/internal/nodes/event.py (1)
id(27-29)backend/apps/owasp/api/internal/nodes/post.py (1)
id(23-25)
backend/apps/owasp/api/internal/nodes/project_health_metrics.py (10)
backend/apps/github/api/internal/nodes/milestone.py (1)
id(31-33)backend/apps/github/api/internal/nodes/pull_request.py (1)
id(26-28)backend/apps/github/api/internal/nodes/issue.py (1)
id(28-30)backend/apps/owasp/api/internal/nodes/common.py (1)
id(13-15)backend/apps/github/api/internal/nodes/release.py (1)
id(29-31)backend/apps/owasp/api/internal/nodes/event.py (1)
id(27-29)backend/apps/github/api/internal/nodes/repository.py (1)
id(44-46)backend/apps/github/api/internal/nodes/organization.py (1)
id(43-45)backend/apps/owasp/api/internal/nodes/post.py (1)
id(23-25)backend/apps/owasp/api/internal/nodes/sponsor.py (1)
id(22-24)
backend/apps/owasp/api/internal/nodes/event.py (10)
backend/apps/github/api/internal/nodes/milestone.py (1)
id(31-33)backend/apps/github/api/internal/nodes/pull_request.py (1)
id(26-28)backend/apps/github/api/internal/nodes/issue.py (1)
id(28-30)backend/apps/owasp/api/internal/nodes/common.py (1)
id(13-15)backend/apps/github/api/internal/nodes/release.py (1)
id(29-31)backend/apps/github/api/internal/nodes/repository.py (1)
id(44-46)backend/apps/github/api/internal/nodes/organization.py (1)
id(43-45)backend/apps/owasp/api/internal/nodes/post.py (1)
id(23-25)backend/apps/owasp/api/internal/nodes/project_health_metrics.py (1)
id(50-52)backend/apps/owasp/api/internal/nodes/sponsor.py (1)
id(22-24)
backend/apps/owasp/api/internal/nodes/post.py (10)
backend/apps/github/api/internal/nodes/milestone.py (1)
id(31-33)backend/apps/github/api/internal/nodes/pull_request.py (1)
id(26-28)backend/apps/github/api/internal/nodes/issue.py (1)
id(28-30)backend/apps/owasp/api/internal/nodes/common.py (1)
id(13-15)backend/apps/github/api/internal/nodes/release.py (1)
id(29-31)backend/apps/owasp/api/internal/nodes/event.py (1)
id(27-29)backend/apps/github/api/internal/nodes/repository.py (1)
id(44-46)backend/apps/github/api/internal/nodes/organization.py (1)
id(43-45)backend/apps/owasp/api/internal/nodes/project_health_metrics.py (1)
id(50-52)backend/apps/owasp/api/internal/nodes/sponsor.py (1)
id(22-24)
backend/apps/github/api/internal/nodes/organization.py (5)
backend/apps/github/api/internal/nodes/milestone.py (1)
id(31-33)backend/apps/github/api/internal/nodes/pull_request.py (1)
id(26-28)backend/apps/github/api/internal/nodes/issue.py (1)
id(28-30)backend/apps/github/api/internal/nodes/release.py (1)
id(29-31)backend/apps/github/api/internal/nodes/repository.py (1)
id(44-46)
🔇 Additional comments (15)
backend/apps/github/api/internal/nodes/pull_request.py (1)
25-28: Add id mapped to node_id — looks goodResolver and typing are correct; aligns with the “stable non-integer id” preference for GitHub entities.
backend/apps/github/api/internal/nodes/release.py (1)
28-31: Confirm non-null/stability of node_id for releasesIf any Release rows lack node_id, the resolver would return null and break client caching assumptions. Please validate data constraints/migrations ensure node_id is always present.
backend/apps/github/api/internal/nodes/issue.py (1)
27-30: LGTM: stable id via node_idMatches the project-wide pattern and satisfies GraphQL clients’ id expectation.
backend/apps/owasp/api/internal/nodes/common.py (1)
12-15: Use of key as id — verify uniqueness and immutabilityPlease confirm the key is guaranteed unique and immutable over the entity’s lifetime (schema constraint or invariant). That keeps client caches consistent.
backend/apps/owasp/api/internal/nodes/event.py (1)
26-29: LGTM: id derived from keyCorrect type/annotation; aligns with the intended stable identifier.
backend/apps/github/api/internal/nodes/milestone.py (1)
30-33: LGTM: id resolves to node_idConsistent with other GitHub nodes and PR goals.
backend/apps/owasp/api/internal/nodes/sponsor.py (1)
21-24: LGTM: id derived from keyMatches stable-id guidance for OWASP entities.
backend/apps/github/api/internal/nodes/organization.py (1)
42-45: LGTM: stable ID via GitHub node_idReturning node_id as the GraphQL ID matches the PR plan and ensures stability for client caching.
backend/tests/apps/github/api/internal/nodes/milestone_test.py (1)
19-19: Test expectation update looks correctIncluding "id" in expected_field_names aligns with the new resolver on MilestoneNode.
backend/tests/apps/github/api/internal/nodes/repository_test.py (1)
28-28: ProjectNode properly exposesidvia its base class
TheGenericEntityNodedefines anidfield resolver (@strawberry.field def id(self) -> strawberry.ID), whichProjectNodeinherits. The test’s expectation for"id"is satisfied—no changes required.backend/tests/apps/owasp/api/internal/nodes/project_health_metrics_test.py (1)
23-23: Test expectation update is fineIncluding "id" here is consistent with the goal of exposing identifiers for caching.
backend/tests/apps/github/api/internal/nodes/issue_test.py (1)
20-20: Test update matches the new IssueNode.idExpecting "id" here is correct given IssueNode now exposes a stable identifier.
backend/tests/apps/github/api/internal/nodes/release_test.py (1)
17-17: ReleaseNode test update looks goodIncluding "id" aligns with the added resolver on the production node.
backend/tests/apps/github/api/internal/nodes/organization_test.py (2)
14-31: Addition of "id" to OrganizationNode fields looks correctThis aligns with the PR objective to expose a stable id for cache/merge correctness.
23-23: Confirmed: OrganizationNode.id correctly maps to node_id and uses strawberry.IDThe
idfield inbackend/apps/github/api/internal/nodes/organization.py(lines 43–45) is annotated asstrawberry.IDand returnsself.node_idas expected. No further changes required.
backend/apps/owasp/api/internal/nodes/project_health_metrics.py
Outdated
Show resolved
Hide resolved
|
After searching a bit, I realized its better to use |
|
|
Updated entities: Note: After commit github:
owasp:
nest:
What is left (not sure about these):
|
rudransh-shrivastava
changed the title
id field to graphql entities
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
backend/apps/nest/api/internal/nodes/api_key.py (1)
3-3: Relay Node adoption: verify that _id uses ApiKey.uuid (stable) rather than the DB PKGood move switching to strawberry.relay.Node. Please confirm:
- The generated _id/Relay ID is derived from ApiKey.uuid (as per PR objective), not the integer PK.
- The GraphQL schema exposes node/nodes entry points if you plan full Relay compatibility.
If _id currently maps to the PK, consider wiring it to uuid for cache stability.
I can help add tests asserting _id resolves to the ApiKey.uuid and adjust the configuration if needed. Do you want me to propose the changes?
Also applies to: 19-19
backend/tests/apps/github/api/internal/nodes/release_test.py (1)
16-16: Test update aligns with Relay Node; consider asserting _id type as wellIncluding "_id" in expected fields matches the Relay Node refactor. Optionally, also assert the type of _id (e.g., str/ID) to catch schema regressions.
backend/tests/apps/nest/api/internal/nodes/api_keys_test.py (1)
17-17: Add a simple type assertion for _id to strengthen the testYou already validate presence; adding a type check will guard against regressions.
fields_map = {f.name: f for f in ApiKeyNode.__strawberry_definition__.fields} + assert fields_map["_id"].type is str or getattr(fields_map["_id"].type, "of_type", None) is str assert fields_map["uuid"].type is uuid.UUID assert fields_map["name"].type is str assert fields_map["is_revoked"].type is bool assert fields_map["created_at"].type is datetime.datetime assert fields_map["expires_at"].type is datetime.datetimebackend/apps/github/api/internal/nodes/pull_request.py (1)
17-17: Ensure PullRequestNode’s _id derives from model.node_id per PR planThe class now inherits strawberry.relay.Node. Please verify the underlying ID source is the stable GitHub node_id (not the DB PK), consistent with the PR’s mapping matrix.
I can add/adjust a test fixture to assert _id == instance.node_id for a PullRequest instance. Want me to draft it?
backend/apps/nest/api/internal/nodes/user.py (1)
3-3: AuthUserNode: verify _id source and naming consistency (login vs username)
- Confirm the Relay/_id is derived from a stable unique attribute (login preferred), not the DB PK.
- The frontend type uses login while this node exposes username; double-check consistency or document the mapping to avoid confusion.
If needed, add an assertion in tests that _id == user.login (or the chosen stable ID).
I can propose a small test ensuring _id maps to login and help adjust the resolver/config if it doesn’t yet.
Also applies to: 10-10
backend/tests/apps/nest/api/internal/nodes/user_test.py (1)
12-12: LGTM; optionally assert _id type for extra safetyIncluding "_id" matches the Relay Node change. You can also assert its type (str/ID) to harden the test if desired.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
backend/apps/github/api/internal/nodes/issue.py(1 hunks)backend/apps/github/api/internal/nodes/milestone.py(1 hunks)backend/apps/github/api/internal/nodes/organization.py(1 hunks)backend/apps/github/api/internal/nodes/pull_request.py(1 hunks)backend/apps/github/api/internal/nodes/release.py(1 hunks)backend/apps/github/api/internal/nodes/repository.py(1 hunks)backend/apps/nest/api/internal/nodes/api_key.py(2 hunks)backend/apps/nest/api/internal/nodes/user.py(1 hunks)backend/apps/owasp/api/internal/nodes/common.py(1 hunks)backend/apps/owasp/api/internal/nodes/event.py(2 hunks)backend/apps/owasp/api/internal/nodes/post.py(2 hunks)backend/apps/owasp/api/internal/nodes/sponsor.py(2 hunks)backend/tests/apps/github/api/internal/nodes/issue_test.py(1 hunks)backend/tests/apps/github/api/internal/nodes/milestone_test.py(1 hunks)backend/tests/apps/github/api/internal/nodes/organization_test.py(1 hunks)backend/tests/apps/github/api/internal/nodes/release_test.py(1 hunks)backend/tests/apps/github/api/internal/nodes/repository_test.py(1 hunks)backend/tests/apps/nest/api/internal/nodes/api_keys_test.py(1 hunks)backend/tests/apps/nest/api/internal/nodes/user_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- backend/apps/owasp/api/internal/nodes/common.py
- backend/tests/apps/github/api/internal/nodes/milestone_test.py
- backend/tests/apps/github/api/internal/nodes/organization_test.py
- backend/tests/apps/github/api/internal/nodes/issue_test.py
- backend/apps/github/api/internal/nodes/issue.py
- backend/tests/apps/github/api/internal/nodes/repository_test.py
- backend/apps/github/api/internal/nodes/repository.py
- backend/apps/owasp/api/internal/nodes/event.py
- backend/apps/github/api/internal/nodes/milestone.py
- backend/apps/owasp/api/internal/nodes/post.py
- backend/apps/owasp/api/internal/nodes/sponsor.py
- backend/apps/github/api/internal/nodes/release.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.
Applied to files:
backend/apps/nest/api/internal/nodes/api_key.py
🧬 Code Graph Analysis (1)
backend/apps/nest/api/internal/nodes/user.py (2)
backend/apps/github/models/user.py (1)
User(19-159)frontend/src/types/user.ts (1)
User(10-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/apps/github/api/internal/nodes/organization.py (1)
39-39: Confirm OrganizationNode’s _id maps to model.node_id (stable), not PKPlease verify the Relay/node ID for OrganizationNode uses the GitHub node_id, per the PR objective. This ensures consistent client-side caching.
|
@arkid15r Hi, pr should be ready for review. |
* add id field * update code * add id field by inheriting from strawberry.relay.Node
* add id field * update code * add id field by inheriting from strawberry.relay.Node
2 participants
Proposed change
Resolves #1976
Note: following changes are only applicable before commit ad618a9bbce8569f8264113156250e743daa67a4
Add
idfield to entities.Used the following fields from the database.
github:
owasp:
What is left (not sure about these):
uuidloginChecklist
make check-testlocally; all checks and tests passed.