feat: Support renames in reference expressions#860
Merged
Conversation
…s, or roles are renamed, authorizer expressions are automatically updated to use the new names
🦋 Changeset detectedLatest commit: fd0e7a3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
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 |
Deploying baseplate-storybook with
|
| Latest commit: |
d37f5ca
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ef898967.baseplate-storybook.pages.dev |
| Branch Preview URL: | https://kingston-eng-1070-support-re.baseplate-storybook.pages.dev |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis pull request adds support for automatically updating authorizer expressions when fields, relations, or roles are renamed. It introduces expression parser infrastructure (specs, refs, validation), implements rename orchestration logic, and integrates these into the definition save/validation pipeline across server and web packages. Changes
Sequence DiagramsequenceDiagram
actor User
participant Web as Web App
participant Server as Server<br/>(Draft Handler)
participant Parser as Expression<br/>Parser
participant Refs as Ref<br/>Orchestrator
participant Schema as Schema<br/>Validator
User->>Web: Update field/role name
Web->>Server: Stage update with old definition
Server->>Server: Load draft with oldRefPayload
Note over Server: oldRefPayload = previous<br/>expression resolution state
Server->>Refs: fixDefinitionRefs(schema, newDef,<br/>{ oldRefPayload })
Refs->>Parser: Parse old expressions
Note over Parser: Extract referenced entities<br/>via getReferencedEntities()
Parser-->>Refs: Dependencies with positions
Refs->>Refs: Compare old vs new<br/>entity IDs (detect renames)
Note over Refs: Old field "x" → New "y"<br/>Maps position ranges to<br/>new names
Refs->>Refs: Apply substring replacements<br/>to expressions in descending<br/>position order
Refs->>Schema: fixRefDeletions(schema,<br/>renamed definition)
Schema-->>Refs: Clean definition
Refs-->>Server: Updated definition<br/>+ modified flag
Server->>Server: Save to project
Server-->>Web: Success
Web-->>User: Field renamed, expressions updated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/project-builder-lib/src/references/expression-types.ts (1)
54-57: Clarify that dependency ranges are[start, end).The new authorizer parser is emitting exclusive end offsets, but this interface only says “end position”. Making the convention explicit here will help future parsers avoid off-by-one replacements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/references/expression-types.ts` around lines 54 - 57, The comment for the dependency range fields is ambiguous about inclusive/exclusive end offsets; update the JSDoc for the start and end properties (the start and end fields in the same interface) to explicitly state the range is half-open [start, end) so callers know end is exclusive; change the existing comment lines to mention "end is exclusive" (or "range is [start, end)"), and ensure any surrounding description refers to that convention so parsers emitting exclusive end offsets are correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/settings.json:
- Line 61: The permission pattern "Bash(git mv:*)" is malformed and should use
space-delimited wildcard syntax; update the entry that contains "Bash(git mv:*)"
to use a space between the command and the wildcard (e.g., "Bash(git mv *)") so
it correctly matches git mv invocations, and ensure the updated pattern follows
the same format as the other Bash(...) entries in the settings file.
In `@packages/project-builder-lib/src/parser/collect-expression-issues.ts`:
- Around line 41-47: Wrap each call to expression.parser.validate inside a
try/catch so a thrown exception from plugin code does not abort the entire loop;
in the catch create a warning object describing the exception and push it into
the same per-expression warnings collection (the one currently assigned to the
variable warnings or whatever is stored on the expression), ensuring you still
continue to the next expression. Specifically modify the loop that iterates over
expressions and calls expression.parser.validate(...) so any error is caught and
converted into a warning attached to that expression (include error.message and
identifying context like expression.value/parser name) instead of rethrowing.
In `@packages/project-builder-lib/src/references/expression-parser-ref.ts`:
- Around line 21-25: The JSDoc example for createExpressionParserRef incorrectly
passes a Zod schema instance to createExpressionParserRef; update the example so
that createExpressionParserRef is called with a schema factory function (e.g.,
pass a function that returns z.string().min(1, 'Expression is required')) when
creating authorizerExpressionRef to match the API signature of
createExpressionParserRef.
In `@packages/project-builder-lib/src/references/fix-expression-renames.ts`:
- Around line 62-117: The current rewrite unconditionally applies replacements
built from oldDefinition which can overwrite edits or resurrect deleted nodes in
newDefinition; change the logic after building updated to first read the current
value at expression.path from newDefinition (or use get(newDefinition,
expression.path)) and only push an update if that current value strictly equals
the original old expression.value (expression.value) — also skip pushing if the
path does not exist in newDefinition to avoid resurrecting deleted nodes;
finally, when applying updates keep the existing set(result, path, value) but
only for updates that passed those checks so you never clobber newer edits or
recreate removed nodes.
---
Nitpick comments:
In `@packages/project-builder-lib/src/references/expression-types.ts`:
- Around line 54-57: The comment for the dependency range fields is ambiguous
about inclusive/exclusive end offsets; update the JSDoc for the start and end
properties (the start and end fields in the same interface) to explicitly state
the range is half-open [start, end) so callers know end is exclusive; change the
existing comment lines to mention "end is exclusive" (or "range is [start,
end)"), and ensure any surrounding description refers to that convention so
parsers emitting exclusive end offsets are correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42f9653e-90de-4f28-b051-ccc4e176d1db
📒 Files selected for processing (45)
.changeset/support-expression-renames.md.claude/settings.jsonpackages/project-builder-lib/src/expression-parsers/authorizer/authorizer-expression-acorn-parser.tspackages/project-builder-lib/src/expression-parsers/authorizer/authorizer-expression-acorn-parser.unit.test.tspackages/project-builder-lib/src/expression-parsers/authorizer/authorizer-expression-ast.tspackages/project-builder-lib/src/expression-parsers/authorizer/authorizer-expression-parser.tspackages/project-builder-lib/src/expression-parsers/authorizer/authorizer-expression-rename.unit.test.tspackages/project-builder-lib/src/expression-parsers/authorizer/authorizer-expression-validator.tspackages/project-builder-lib/src/expression-parsers/authorizer/authorizer-expression-validator.unit.test.tspackages/project-builder-lib/src/expression-parsers/authorizer/authorizer-expression-visitor.tspackages/project-builder-lib/src/expression-parsers/authorizer/index.tspackages/project-builder-lib/src/expression-parsers/register-core-module.tspackages/project-builder-lib/src/index.tspackages/project-builder-lib/src/parser/collect-definition-issues.tspackages/project-builder-lib/src/parser/collect-definition-issues.unit.test.tspackages/project-builder-lib/src/parser/collect-expression-issues.tspackages/project-builder-lib/src/parser/collect-expression-issues.unit.test.tspackages/project-builder-lib/src/references/expression-parser-ref.tspackages/project-builder-lib/src/references/expression-parser-spec.tspackages/project-builder-lib/src/references/expression-types.tspackages/project-builder-lib/src/references/extend-parser-context-with-refs.tspackages/project-builder-lib/src/references/extract-definition-refs.tspackages/project-builder-lib/src/references/fix-definition-refs.tspackages/project-builder-lib/src/references/fix-expression-renames.tspackages/project-builder-lib/src/references/fix-expression-renames.unit.test.tspackages/project-builder-lib/src/references/index.tspackages/project-builder-lib/src/references/parse-schema-with-references.tspackages/project-builder-lib/src/schema/creator/schema-creator.tspackages/project-builder-lib/src/schema/models/authorizer/authorizer-expression-parser.tspackages/project-builder-lib/src/schema/models/authorizer/authorizer-expression-ref.tspackages/project-builder-lib/src/schema/models/authorizer/authorizer.tspackages/project-builder-lib/src/schema/models/authorizer/index.tspackages/project-builder-lib/src/testing/expression-stub-parser.test-helper.tspackages/project-builder-lib/src/testing/expression-warning-parser.test-helper.tspackages/project-builder-lib/src/testing/parser-context.test-helper.tspackages/project-builder-lib/src/testing/project-definition-container.test-helper.tspackages/project-builder-server/src/actions/definition/definition-test-fixtures.test-helper.tspackages/project-builder-server/src/actions/definition/draft-lifecycle.int.test.tspackages/project-builder-server/src/actions/definition/draft-session.tspackages/project-builder-server/src/actions/definition/stage-update-entity.action.tspackages/project-builder-server/src/actions/definition/validate-draft.tspackages/project-builder-server/src/core-modules/index.tspackages/project-builder-web/src/app/project-definition-provider/project-definition-provider.tsxpackages/project-builder-web/src/core-modules/index.tsplugins/plugin-payments/src/stripe/core/schema/schema-issue-checker.ts
💤 Files with no reviewable changes (1)
- packages/project-builder-lib/src/schema/models/authorizer/authorizer-expression-parser.ts
packages/project-builder-lib/src/parser/collect-expression-issues.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+62
to
+117
| for (const expression of oldExpressions) { | ||
| // Parse and resolve entities against the OLD definition where old names still exist | ||
| const parseResult = expression.parser.parse( | ||
| expression.value, | ||
| oldDefinition, | ||
| ); | ||
| if (!parseResult.success) { | ||
| // Don't touch broken expressions | ||
| continue; | ||
| } | ||
|
|
||
| const refs = expression.parser.getReferencedEntities( | ||
| expression.value, | ||
| parseResult, | ||
| oldDefinition, | ||
| expression.resolvedSlots, | ||
| ); | ||
|
|
||
| // Build replacements for renamed entities | ||
| const replacements: { start: number; end: number; newValue: string }[] = []; | ||
| for (const ref of refs) { | ||
| const newName = renames.get(ref.entityId); | ||
| if (newName !== undefined) { | ||
| replacements.push({ | ||
| start: ref.start, | ||
| end: ref.end, | ||
| newValue: newName, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| if (replacements.length === 0) { | ||
| continue; | ||
| } | ||
|
|
||
| // Sort by position descending so earlier replacements don't shift later positions | ||
| replacements.sort((a, b) => b.start - a.start); | ||
|
|
||
| let updated = expression.value as string; | ||
| for (const { start, end, newValue } of replacements) { | ||
| updated = updated.slice(0, start) + newValue + updated.slice(end); | ||
| } | ||
|
|
||
| modified = true; | ||
| // Update at the same path in the NEW definition | ||
| updates.push({ path: expression.path, value: updated }); | ||
| } | ||
|
|
||
| if (!modified) { | ||
| return { value: newDefinition, modified: false }; | ||
| } | ||
|
|
||
| const result = structuredClone(newDefinition) as object; | ||
| for (const { path, value } of updates) { | ||
| set(result, path, value); | ||
| } |
There was a problem hiding this comment.
Prevent stale expression rewrites and path resurrection during rename application.
Current logic rewrites from oldRefPayload text and unconditionally sets by old path. If the expression was edited or removed in the new definition, this can clobber newer content or recreate deleted nodes.
🔧 Proposed fix
-import { set } from 'es-toolkit/compat';
+import { get, set } from 'es-toolkit/compat';
@@
for (const expression of oldExpressions) {
+ const currentValue = get(newDefinition as object, expression.path);
+ if (typeof currentValue !== 'string') {
+ // Expression was removed or shape changed in new definition.
+ continue;
+ }
+
// Parse and resolve entities against the OLD definition where old names still exist
const parseResult = expression.parser.parse(
- expression.value,
+ currentValue,
oldDefinition,
);
@@
const refs = expression.parser.getReferencedEntities(
- expression.value,
+ currentValue,
parseResult,
oldDefinition,
expression.resolvedSlots,
);
@@
- let updated = expression.value as string;
+ let updated = currentValue;
for (const { start, end, newValue } of replacements) {
updated = updated.slice(0, start) + newValue + updated.slice(end);
}
- modified = true;
- // Update at the same path in the NEW definition
- updates.push({ path: expression.path, value: updated });
+ if (updated !== currentValue) {
+ modified = true;
+ // Update at the same path in the NEW definition
+ updates.push({ path: expression.path, value: updated });
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-lib/src/references/fix-expression-renames.ts` around
lines 62 - 117, The current rewrite unconditionally applies replacements built
from oldDefinition which can overwrite edits or resurrect deleted nodes in
newDefinition; change the logic after building updated to first read the current
value at expression.path from newDefinition (or use get(newDefinition,
expression.path)) and only push an update if that current value strictly equals
the original old expression.value (expression.value) — also skip pushing if the
path does not exist in newDefinition to avoid resurrecting deleted nodes;
finally, when applying updates keep the existing set(result, path, value) but
only for updates that passed those checks so you never clobber newer edits or
recreate removed nodes.
kingston
deleted the
kingston/eng-1070-support-renames-in-reference-expressions
branch
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.
Summary by CodeRabbit