Repository navigation
[#20967] fix(store-api): read all fields of the compressed _criteria parameter - #162
Closed
BrocksiNet wants to merge 13 commits into
Closed
BrocksiNet wants to merge 13 commits into
BrocksiNet wants to merge 13 commits into
Conversation
…re/shopware into fix/store-api-get-criteria-parity
…e under the right key
…re/shopware into fix/store-api-get-criteria-parity
BrocksiNet
added
mirror
Owner
Author
|
@coderabbitai review |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (36)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughStore API GET requests can now read route parameters from compressed ChangesStore API compressed criteria
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant StoreApiRequest
participant CompressedCriteriaRequestListener
participant RequestCriteriaBuilder
StoreApiRequest->>CompressedCriteriaRequestListener: Send GET request with _criteria
CompressedCriteriaRequestListener->>CompressedCriteriaRequestListener: Decode criteria and update query parameters
CompressedCriteriaRequestListener->>RequestCriteriaBuilder: Pass expanded request query
Merge Risk: ⚪ Minimal · up to The changed request behavior has coverage across the affected routes, and no actionable merge blocker is established. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to A shared input change affects many Store API routes. The inspected identity and cache controls limit apparent exposure, but consistent validation and authorization of the newly readable parameters across routes remains unconfirmed. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 22.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 100 functions across 26 files. (10 skipped: 10 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
✅ Action performedReview finished.
|
BrocksiNet
added
review-done
Owner
Author
|
Upstream PR no longer requests a review from you - closing this mirror. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@mkucmustrunkat merge-baseb5e77c66bffd6d9653889efaOriginal description
1. Why is this change necessary?
Store API clients send cacheable reads as GET and put the request body into
_criteria. The cache ADR asks SDKs to pick GET or POST automatically, so both have to return the same result. Today they do not.What it fixes
_criteriaare ignored on some routes:limit,includesandexcludeson/category,/cms,/landing-page,/media,/find-variantand the cart, andlimiton/search. The routes answer200with the wrong data.#19863fixed the same problem for routes with aCriteriaargument./search-suggestanswers400whensearchis inside_criteria, although/searchreads it from there.sw-include-search-infoheader is ignored when_criteriais present.includesthat is not an array gives500instead of400.What it extends
_criteriatoo, for exampleslots,depth,buildTree,options,switchedGroupandonlyAvailable. Before,_criteriameant the criteria only, and these had to be sent as plain query parameters. Inside_criteriathey were ignored without an error, so/find-variantreturned a different variant than asked for.#13643already did this for the listing parameters (p,order, filters).What it enables
2. What does this change do, exactly?
CompressedCriteriaRequestListener: on every Store API GET request, it copies the fields of_criteriainto the query parameters before the controller runs. So_criteriaworks as a compressed query string.#13643did the same for product listings._criteriawins over a plain query parameter with the same name. Plain parameters keep working._criteriaare read as strings, as in a query string, and a field set tonullcounts as not sent, as in a POST body. The criteria keep their JSON types.RequestCriteriaBuilderapplies plain query parameters next to_criteriaand respectssw-include-search-info. This also affects the Admin API GET list and detail routes.includesandexcludesare validated inStoreApiResponseListenerandScriptStoreApiRoute.CompressedCriteriaListingProcessorfrom#13643now skips Store API requests. There the listener already copied the fields as query-string values, and the processor overwrote them again with their JSON types. Outside the Store API the processor works as before. This is intended: the PR changes nothing outside the Store API, so there is no BC break. Deprecating the processor can follow in a separate PR.Limits
_criteriachange. Plain GET and POST requests behave as before.CACHE_REWORKor v6.8. Until then, the gain is correct GET results.manufacturer-filter), still do not work over GET. This is a follow-up._criterianow means more than criteria. This is a public contract that core has to keep.3. Describe each step to reproduce the issue or behaviour.
POST /store-api/category/{id}with the body{"limit":2,"includes":{"category":["id","cmsPage"]}}.node -e 'console.log(require("zlib").gzipSync(JSON.stringify({limit:2,includes:{category:["id","cmsPage"]}})).toString("base64url"))'GET /store-api/category/{id}?_criteria=<encoded>.Before: GET returns the full category with the default number of products. After: GET returns the same as POST.
4. Please link to the relevant issues (if any).
limitinside_criteriaon/search#19863- fixed the same problem for routes with aCriteriaargument#13643- introduced the approach this change extends5. Checklist
RELEASE_INFO-6.<major>.mdunder “Upcoming” for informational changes, including the consequences of the change and how it affects external developers.UPGRADEsection inUPGRADE-6.<next-major>.mdfor breaking changes (what/why/impact/how to adapt).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
_criteriaacross routes, allowing request fields to be supplied in the URL. Criteria values take precedence over matching query parameters, while other query parameters remain in effect.Bug Fixes
includesorexcludesvalues and malformed compressed criteria now return a 400 response instead of being ignored or causing a server error.