-
-
Notifications
You must be signed in to change notification settings - Fork 247
Fix writeOnly schema properties missing from request types in nested schemas #2793
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
🦋 Changeset detectedLatest commit: 7d1c475 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for GitHub.
💡 Enable Vercel Agent with $100 free credit for automated AI reviews |
Co-authored-by: mrlubos <[email protected]>
Co-authored-by: mrlubos <[email protected]>
Co-authored-by: mrlubos <[email protected]>
@copilot you need to run |
Co-authored-by: mrlubos <[email protected]>
Updated all test snapshots with |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2793 +/- ##
==========================================
- Coverage 29.17% 29.15% -0.02%
==========================================
Files 391 391
Lines 38111 38146 +35
Branches 2279 2279
==========================================
+ Hits 11120 11123 +3
- Misses 26961 26993 +32
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
commit: |
@copilot most of these changes look good! The only diff I am confused about is in the |
You're absolutely right - this is a regression. I've traced through the logic and I believe the issue is that when walking through Let me revert this specific change and investigate a more targeted fix. The core fix (following $refs to propagate context) is correct for the reported issue, but we need to ensure it doesn't cause regressions in edge cases like this. I'll update the snapshot for this file to use
Fixed in commit c9f4e21. The regression was caused by schemas being walked multiple times from different paths. Added a global visited set to ensure each schema is only walked once (from the first context it's encountered in), which prevents conflicting updates. The |
Add global visited set to prevent schemas from being walked multiple times when following $refs. This fixes the regression where BroadcastedResponses was incorrectly using LocationResponseWritable instead of LocationResponse. The fix ensures that each schema is only walked once (from the first context it's encountered in), which prevents conflicting updates when the same schema is referenced from multiple paths. Co-authored-by: mrlubos <[email protected]>
@copilot fix the issues caught in CI checks |
CI checks fixed (commit: will be in next push):
Known issue: 1 test failure with "custom read-only and write-only naming" when a schema is used in both request and response contexts. This is an edge case where the global visited set (needed to prevent the Dutchie regression) prevents schemas from being walked multiple times. Investigating solutions that handle both cases. |
Summary
Fixed issue where writeOnly schema properties were missing from request types when referenced through intermediate schemas.
CI Status:
Root Cause: When a schema like
CreateItemRequest
references another schemaPayload
that haswriteOnly
properties, theupdateRefsInSpec
function would walk throughCreateItemRequest
with null context, causing $refs inside it to default to the read variant.Solution:
Known Issue: The "custom read-only and write-only naming" test fails when a schema is used in both request and response contexts with custom naming. This is because the global visited set prevents the schema from being walked twice. Further investigation needed to support this edge case without breaking the Dutchie spec.
Tests Added:
transforms-read-write-nested.yaml
: Tests writeOnly properties in nested request schemastransforms-read-write-response.yaml
: Tests readOnly properties in nested response schemasSnapshot Updates: Updated 115 test snapshots to reflect the fix.
Original prompt
Fixes #2792
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.