-
Notifications
You must be signed in to change notification settings - Fork 18
feat(vtex): add schema parameter to Masterdata loaders and actions #1502
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
feat(vtex): add schema parameter to Masterdata loaders and actions #1502
Conversation
WalkthroughAdds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
Tagging OptionsShould a new tag be published when this PR is merged?
|
b5d2b63 to
d4dd44f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
vtex/actions/masterdata/createDocument.ts(3 hunks)vtex/actions/masterdata/updateDocument.ts(3 hunks)vtex/loaders/masterdata/searchDocuments.ts(3 hunks)vtex/utils/client.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
vtex/actions/masterdata/updateDocument.ts
[error] 43-43: TS2353: Object literal may only specify known properties, and '_schema' does not exist in type '{ acronym: URLPatternParam; }'.
[error] 47-47: TS2353: Object literal may only specify known properties, and '_schema' does not exist in type '{ acronym: URLPatternParam; } & { id: URLPatternParam; }'.
vtex/loaders/masterdata/searchDocuments.ts
[error] 63-63: TS2353: Object literal may only specify known properties, and '_schema' does not exist in type '{ acronym: URLPatternParam; } & { _fields?: string | undefined; _where?: string | undefined; _sort?: string | undefined; }'.
vtex/actions/masterdata/createDocument.ts
[error] 39-39: TS2353: Object literal may only specify known properties, and '_schema' does not exist in type '{ acronym: URLPatternParam; }'.
🔇 Additional comments (5)
vtex/actions/masterdata/updateDocument.ts (1)
5-14: Optionalschemaprop wiring looks correctAdding
schema?: stringtoPropsand threading it through the destructuring keeps the action backwards compatible while exposing schema selection where needed. No issues here.Also applies to: 28-29
vtex/loaders/masterdata/searchDocuments.ts (2)
19-22:schemaprop and documentation are consistent with Masterdata usageThe new optional
schemafield onPropsplus its description look good and align with VTEX Masterdata’s_schemaquery parameter. No changes needed here.
54-64: Verify that_schemais not a valid parameter for the search endpoint and that TS2353 error occursThe proposed fix assumes the route's type signature excludes
_schema, causing excess property check failures. Verify this is the actual type definition and that the conditional approach suggested (creatingbaseParamsand conditionally spreading_schema) is the correct fix rather than updating the route type definition itself.vtex/utils/client.ts (1)
291-297:_schemasearchParams mapping for POST documents is aligned with new behaviorAdding an optional
_schemaundersearchParamsfor"POST /api/dataentities/:acronym/documents"matches the newschemasupport while remaining backwards compatible for existing callers that don’t set it. Looks good; just ensure this interface corresponds to the client actually used for Masterdata document creation.vtex/actions/masterdata/createDocument.ts (1)
5-10: Optionalschemaon create props is safe and consistentExposing
schema?: stringonPropsand threading it through the destructuring is backwards compatible and consistent with the other Masterdata operations.Also applies to: 24-25
94d5de5 to
1e237c4
Compare
1e237c4 to
2564730
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
vtex/loaders/masterdata/searchDocuments.ts (1)
19-22: LGTM! Optional enhancement: consider expanding documentation.The addition of the optional
schemaparameter is implemented correctly and maintains backward compatibility.For improved developer experience, consider adding more context to the JSDoc:
/** - * @description Schema of the data entity. + * @description Schema name for the data entity. Use this when querying a specific schema version or custom schema. If not provided, the default schema is used. */ schema?: string;vtex/utils/openapi/vcs.openapi.gen.ts (1)
868-873: JSDoc for_schemaparameter is inaccurate on five endpointsThe reused JSDoc stating "This field is required when using
_whereor_fieldsquery parameters" appears on endpoints that don't expose those parameters:
POST /api/dataentities/:acronym/documents(line 868)PATCH /api/dataentities/:acronym/documents(line 903)PUT /api/dataentities/:acronym/documents/:id(line 998)DELETE /api/dataentities/:acronym/documents/:id(line 1032)PATCH /api/dataentities/:acronym/documents/:id(line 1059)Only
GET /api/dataentities/:acronym/documents/:idactually includes_fieldsin itssearchParams, where the JSDoc is partially justified. Clarify the documentation to either remove the conditional reference on endpoints that don't expose these parameters, or rephrase generically. Correct this in the OpenAPI source or generator rather than in the generated TypeScript.vtex/utils/openapi/vcs.openapi.json (1)
974-984: Clarify therequiredfield: parameter is conditionally required but markedrequired: false.The parameter description states: "This field is required when using
_whereor_fieldsquery parameters," yet it's marked asrequired: false. This creates ambiguity for API consumers about when the parameter is actually mandatory.OpenAPI does not natively support conditional required fields. Consider one of these approaches:
- Update the description to explicitly state when the parameter is optional vs. required at the schema documentation level.
- Mark as required: true if the parameter should always be provided for proper schema resolution.
- Document in prose (outside the spec) that the parameter is strongly recommended for
_where/_fieldsqueries to avoid endpoint fallbacks.Clarify the intent and update the parameter definition and/or documentation accordingly.
Also applies to: 1572-1582
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
vtex/actions/masterdata/createDocument.ts(3 hunks)vtex/actions/masterdata/updateDocument.ts(3 hunks)vtex/loaders/masterdata/searchDocuments.ts(3 hunks)vtex/utils/client.ts(1 hunks)vtex/utils/openapi/vcs.openapi.gen.ts(7 hunks)vtex/utils/openapi/vcs.openapi.json(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- vtex/actions/masterdata/createDocument.ts
- vtex/utils/client.ts
- vtex/actions/masterdata/updateDocument.ts
🔇 Additional comments (2)
vtex/loaders/masterdata/searchDocuments.ts (1)
54-54: No action required. The HTTP client implementation inutils/http.ts(lines 201-202) explicitly filters out undefined values before constructing the URL search parameters, so_schema: undefinednever reaches the VTEX API.vtex/utils/openapi/vcs.openapi.gen.ts (1)
733-751: GET search:_schemaquery param addition looks correctThe new optional
_schema?: stringinsearchParamsfor"GET /api/dataentities/:acronym/search"is consistent with Master Data v1 usage and the JSDoc matches the presence of_whereand_fields. This should unblock schema-aware search callers without breaking existing ones.
Some VTEX Masterdata queries were failing because the corresponding loaders and actions in DecoCX did not receive the required
schemaparameter. Without explicitly passing the schema, certain endpoints fall back to defaults that do not match our use cases, resulting in inconsistent behavior or empty responses.This PR adds support for an optional
schemaparameter across all VTEX Masterdata loaders and actions. When provided, the value is forwarded to the underlying Masterdata client, ensuring that schema-specific queries work as expected.Changes
schemaargument to Masterdata loaders and actionsWhy this is needed
Some Masterdata data sources rely on custom schemas. Without the
schemaparameter, those queries silently fail or return incomplete results. Explicit schema forwarding ensures correct resolution and aligns the integration with VTEX API requirements.Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.