Don't expose foreign keys as attributes in openAPI spec #1856
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/plugins/openapi/src/rest-generator.ts (1)
871-874: LGTM! Consider adding a comment for clarity.The implementation correctly excludes foreign key fields from being exposed as attributes in the OpenAPI specification, which aligns with the PR objective. This is the right approach as foreign keys should be treated as relationships rather than attributes.
Consider adding a brief comment explaining why foreign keys are excluded:
if (isForeignKeyField(field)) { - // foreign keys are not exposed as attributes + // Skip foreign keys as they should be exposed as relationships rather than attributes + // to maintain proper REST/JSON:API resource relationships continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
packages/plugins/openapi/tests/baseline/rest-3.0.0.baseline.yamlis excluded by!**/*.yamlpackages/plugins/openapi/tests/baseline/rest-3.1.0.baseline.yamlis excluded by!**/*.yaml
📒 Files selected for processing (1)
packages/plugins/openapi/src/rest-generator.ts(1 hunks)
🔇 Additional comments (1)
packages/plugins/openapi/src/rest-generator.ts (1)
871-874: Verify the impact of this breaking change.
Removing foreign keys from attributes is a breaking change that could affect existing API clients. While this change is correct and aligns with REST/JSON:API best practices, we should ensure proper communication of this change.
Let's identify affected models and their foreign key fields:
Consider:
- Adding a breaking change notice in the changelog
- Updating API documentation to reflect this change
- Communicating to API consumers about migrating to using relationship endpoints
✅ Verification successful
Let me gather more specific information about the impact by searching for actual foreign key field usage in API clients and tests.
This will help us:
- Find actual usage of foreign key fields in tests
- Identify existing OpenAPI test cases that might need updates
- Locate any documentation that should be updated with breaking change information
Let me check the test baselines to understand how foreign keys are currently handled in the OpenAPI spec.
This will help us:
- Understand the current OpenAPI spec structure for relationships
- See how foreign keys are currently handled in test cases
Based on the analysis of the OpenAPI spec and test files, I can now provide a final response:
Breaking change is aligned with JSON:API specification and properly handled
The removal of foreign keys from attributes is a correct implementation that aligns with JSON:API specification. The analysis shows that:
- Relationship fields are properly exposed through dedicated relationship endpoints (e.g.,
/user/{id}/relationships/posts) - The OpenAPI spec correctly documents relationship access through both relationship links and related resource links
- Test cases demonstrate proper handling of relationships through dedicated endpoints rather than as attributes
The change is safe because:
- Foreign key fields are still accessible through relationship endpoints
- The OpenAPI spec provides clear documentation for relationship access
- Test baselines validate the correct JSON:API structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all models with foreign key fields to assess the impact
# Expected results: List of models and their foreign key fields that will no longer be exposed as attributes
# Search for model definitions with foreign key fields
ast-grep --pattern 'model $MODEL {
$$$
$FIELD $TYPE @relation($$$)
$$$
}'
# Search for potential API clients using these fields
rg -l "\.attributes\."
Length of output: 1614
Script:
#!/bin/bash
# Search for specific foreign key field access patterns in tests and API clients
rg -A 2 "\.attributes\.[a-zA-Z]+Id" packages/plugins/openapi/tests/ packages/server/tests/
# Check OpenAPI test cases and assertions specifically
rg -A 5 "openapi|swagger" packages/plugins/openapi/tests/
# Look for any documentation or migration guides
fd -e md . | xargs rg -l "breaking|migration|upgrade"
Length of output: 24813
Script:
#!/bin/bash
# Check the baseline files for foreign key handling
cat packages/plugins/openapi/tests/baseline/rest-3.1.0.baseline.yaml
# Look for relation field definitions in the schema
rg "@relation" -A 2 packages/plugins/openapi/tests/
Length of output: 130969
There was a problem hiding this comment.
Hi @thomassnielsen , thanks for continue improving this. Please see my comments.
These attributes are handled as relationships, and as such don't need to be exposed as attributes.