feat: new differ v4#479
Open
bzp2010 wants to merge 5 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughIntroduces ChangesDifferV4 Implementation and Call-site Migration
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (diff/sync/validate)
participant Differ as Differ (DifferV4)
participant RESOURCE_DIFFER_META as RESOURCE_DIFFER_META
participant Schema as Zod Schema Metadata
participant Backend as Backend
CLI->>Differ: Differ.diff(local, remote, defaultValue)
Differ->>RESOURCE_DIFFER_META: iterate resource entries
RESOURCE_DIFFER_META->>Schema: readFieldMeta(schema)
Schema-->>RESOURCE_DIFFER_META: FieldMeta per field
loop each ResourceType in RESOURCE_DIFFER_META
Differ->>Differ: extractTuples(config, meta)
Differ->>Differ: diffResource(localTuples, remoteTuples, meta)
note over Differ: handleCreate / handleUpdate / handleDelete
Differ->>Differ: applyAtomicStrips, mergeDefault, diffPlugins
Differ->>Differ: postprocessSubEvent for nested resources
end
Differ->>Differ: flatten ONLY_SUB_EVENTS, sort by order table
Differ-->>CLI: sorted Event[]
CLI->>Backend: backend.sync(diff) / backend.validate(events)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
libs/sdk/src/core/differ.ts (1)
68-73: ⚡ Quick winInclude
configKeyinFieldMetafor MAP fields to avoid downstream hardcoding.The schema already carries
configKeymetadata (e.g., consumer credentials), but theFieldMetatype drops it. That forces manual mapping later and can drift from schema annotations.💡 Proposed refactor
export type FieldMeta = - | { listType: typeof FieldListType.MAP; listMapKey: string; nested?: boolean } + | { + listType: typeof FieldListType.MAP; + listMapKey: string; + nested?: boolean; + configKey?: keyof InternalConfiguration; + } | { listType: typeof FieldListType.OBJECT_MAP } | { listType: typeof FieldListType.ATOMIC; strip?: boolean } | { listType: typeof FieldListType.ARRAY; stripItemFields?: string[] };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/sdk/src/core/differ.ts` around lines 68 - 73, The FieldMeta type variant for FieldListType.MAP is missing the configKey field that carries metadata in the schema, which forces downstream manual mapping. Add a configKey property to the MAP variant of FieldMeta (the one with listType: typeof FieldListType.MAP; listMapKey: string; nested?: boolean) to capture and preserve the schema annotation and prevent downstream hardcoding.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/differ/src/differv4.ts`:
- Around line 84-93: The debug log in the differ.logger?.debug call is logging
full config and resource payloads (local, remote, defaultValue) which can expose
sensitive information like tokens, auth headers, or private keys. Replace these
full object logs with safer summaries such as counts of keys, array lengths, or
hashes instead of the complete objects. Keep the transactionId and message
fields as they are safe to log. Apply this same fix to the other similar logging
statements in the file at the locations mentioned in the comment (lines 351-363
and 445-455).
- Around line 494-505: The isObjectButNotArray function incorrectly returns true
for null values since typeof null === 'object' in JavaScript, causing the
mergeDefault method to recursively call itself with null as the defaults
parameter, which then fails when trying to iterate entries on null. Fix this by
adding a null check to the isObjectButNotArray function definition so it
explicitly excludes null from being considered as an object (add val !== null to
the condition before checking typeof val === 'object').
In `@libs/sdk/src/core/schema.ts`:
- Line 367: The plugins field in the consumer group schema is currently
required, which breaks compatibility with existing configs that omit the
consumer_group.plugins setting. Locate the plugins field definition in the
consumer group schema (the line with pluginsSchema.meta({ listType:
FieldListType.OBJECT_MAP })) and add the appropriate optional modifier (likely
.optional()) to make the plugins field optional, consistent with other resource
schemas.
---
Nitpick comments:
In `@libs/sdk/src/core/differ.ts`:
- Around line 68-73: The FieldMeta type variant for FieldListType.MAP is missing
the configKey field that carries metadata in the schema, which forces downstream
manual mapping. Add a configKey property to the MAP variant of FieldMeta (the
one with listType: typeof FieldListType.MAP; listMapKey: string; nested?:
boolean) to capture and preserve the schema annotation and prevent downstream
hardcoding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8767be4f-5e29-4ad6-b622-d2e3e8599729
📒 Files selected for processing (24)
apps/cli/src/command/dump.command.tsapps/cli/src/command/utils.spec.tsapps/cli/src/command/utils.tsapps/cli/src/server/sync.tsapps/cli/src/server/validate.tsapps/cli/src/tasks/diff.tslibs/backend-api7/e2e/validate.e2e-spec.tslibs/backend-apisix-standalone/e2e/cache.e2e-spec.tslibs/backend-apisix-standalone/e2e/resources/service-inline-upstream.e2e-spec.tslibs/backend-apisix-standalone/e2e/validate.e2e-spec.tslibs/backend-apisix/e2e/validate.e2e-spec.tslibs/backend-apisix/src/transformer.tslibs/differ/src/differv3.tslibs/differ/src/differv4.tslibs/differ/src/index.tslibs/differ/src/test/basic.spec.tslibs/differ/src/test/consumer.spec.tslibs/differ/src/test/custom-id.spec.tslibs/differ/src/test/service-upstream.spec.tslibs/differ/src/test/upstream.spec.tslibs/differ/src/test/usecase.spec.tslibs/sdk/src/core/differ.tslibs/sdk/src/core/resource.tslibs/sdk/src/core/schema.ts
💤 Files with no reviewable changes (2)
- libs/differ/src/differv3.ts
- libs/backend-apisix/src/transformer.ts
bzp2010
added
test/api7
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
1 participant
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.
Description
Refactor differ v4, no major changes were made, and it passed the old test cases.
Checklist
Summary by CodeRabbit
Release Notes
idfields by default (kept only when--with-idis provided).idundermetadata.pluginsunder consumer groups as optional.