[draft/experimental] Supporting comments in DSL #528
+2,297
−84
Conversation
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
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request introduces a comprehensive comment-preservation subsystem for OpenFGA's DSL transformer across Go and TypeScript. It adds comment tracking, extraction, and injection capabilities to enable comments to survive round-trip transformations between DSL and JSON representations, with corresponding test coverage and sample data. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser
participant CommentTracker
participant JSONTransformer
Client->>Parser: ParseDSLWithOptions(source, preserveComments=true)
Parser->>CommentTracker: NewCommentTracker(source)
CommentTracker->>CommentTracker: parseComments()<br/>(extract preceding & inline)
Parser->>Parser: Parse DSL with ANTLR
Parser->>Parser: Attach CommentTracker to listener
Parser->>Parser: SetTypeComments, SetRelationComments,<br/>SetConditionComments via line mapping
Parser-->>Client: listener + typeComments,<br/>conditionComments, modelComments
Client->>JSONTransformer: TransformDSLToJSONWithComments(source)
JSONTransformer->>JSONTransformer: Parse DSL → obtain comments metadata
JSONTransformer->>JSONTransformer: Marshal proto to JSON
JSONTransformer->>JSONTransformer: Inject model_comments,<br/>type comments, relation comments,<br/>condition comments into metadata
JSONTransformer-->>Client: JSON string with embedded comments
sequenceDiagram
participant Client
participant JSONParser
participant JSONTransformer
participant DSLBuilder
Client->>JSONParser: TransformJSONStringToDSLWithComments(jsonString)
JSONParser->>JSONParser: Unmarshal JSON to<br/>JSONModelWithComments
JSONParser->>JSONParser: Extract comments from metadata.model_comments,<br/>type_definitions[].metadata.comments,<br/>conditions[].metadata.comments
JSONParser->>JSONTransformer: transformJSONProtoToDSLWithComments(proto, commentsModel)
JSONTransformer->>DSLBuilder: Format model-level comments
JSONTransformer->>DSLBuilder: parseTypeWithComments(typeDef, typeComments)
JSONTransformer->>DSLBuilder: parseRelationWithComments(relation, relationComments)
JSONTransformer->>DSLBuilder: parseConditionsWithComments(conditions, conditionComments)
DSLBuilder->>DSLBuilder: formatCommentLines, formatInlineComment
DSLBuilder-->>JSONTransformer: DSL sections with comments
JSONTransformer-->>Client: Complete DSL string with<br/>preserved comments
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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. 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 |
|
Related Documentation Checked 6 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
poovamraj
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds bidirectional comment preservation between DSL and JSON formats for OpenFGA authorization models. Comments can now survive round-trip transformations (DSL → JSON → DSL), addressing the issue where documentation and context were previously lost during format conversions.
Changes:
- Introduced CommentTracker infrastructure in both Go and TypeScript to capture and associate comments with AST elements during parsing
- Extended JSON schema with optional metadata fields for storing comments at model, type, relation, and condition levels
- Implemented new transformation functions that preserve comments:
transformDSLToJSONWithCommentsandtransformJSONStringToDSLWithCommentsin both languages
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/go/transformer/comments.go | New CommentTracker infrastructure for tracking comments in DSL source |
| pkg/go/transformer/dsltojson.go | Modified to extract and embed comments during DSL to JSON conversion |
| pkg/go/transformer/jsontodsl.go | Modified to emit comments when converting JSON to DSL |
| pkg/go/transformer/comments_test.go | Comprehensive tests for Go comment preservation functionality |
| pkg/js/transformer/dsltojson.ts | Added CommentTracker class and comment extraction during DSL parsing |
| pkg/js/transformer/jsontodsl.ts | Added comment emission when converting JSON to DSL |
| pkg/js/tests/comments.test.ts | Comprehensive tests for TypeScript comment preservation functionality |
| pkg/js/package-lock.json | Cleanup of duplicate/optional peer dependency entries for babel-jest packages |
| tests/data/transformer/200-comment-preservation/* | Test data files demonstrating comment preservation |
Files not reviewed (1)
- pkg/js/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+188
to
+229
| // SetRelationComments sets comments for a relation within a type. | ||
| func (ct *CommentTracker) SetRelationComments(typeName, relationName string, lineNum int) { | ||
| commentInfo := ct.GetCommentInfoForLine(lineNum) | ||
| if commentInfo == nil { | ||
| return | ||
| } | ||
|
|
||
| if ct.typeComments[typeName] == nil { | ||
| ct.typeComments[typeName] = &CommentsMetadata{} | ||
| } | ||
| if ct.typeComments[typeName].RelationComments == nil { | ||
| ct.typeComments[typeName].RelationComments = make(map[string]*CommentInfo) | ||
| } | ||
| ct.typeComments[typeName].RelationComments[relationName] = commentInfo | ||
| } | ||
|
|
||
| // SetConditionComments sets comments for a condition. | ||
| func (ct *CommentTracker) SetConditionComments(conditionName string, lineNum int) { | ||
| commentInfo := ct.GetCommentInfoForLine(lineNum) | ||
| if commentInfo == nil { | ||
| return | ||
| } | ||
| ct.condComments[conditionName] = commentInfo | ||
| } | ||
|
|
||
| // GetTypeComments returns the comments metadata for a type. | ||
| func (ct *CommentTracker) GetTypeComments(typeName string) *CommentsMetadata { | ||
| return ct.typeComments[typeName] | ||
| } | ||
|
|
||
| // GetRelationComments returns comments for a relation. | ||
| func (ct *CommentTracker) GetRelationComments(typeName, relationName string) *CommentInfo { | ||
| if ct.typeComments[typeName] == nil { | ||
| return nil | ||
| } | ||
| return ct.typeComments[typeName].RelationComments[relationName] | ||
| } | ||
|
|
||
| // GetConditionComments returns comments for a condition. | ||
| func (ct *CommentTracker) GetConditionComments(conditionName string) *CommentInfo { | ||
| return ct.condComments[conditionName] | ||
| } |
There was a problem hiding this comment.
The methods SetTypeComments, SetRelationComments, SetConditionComments, GetTypeComments, GetRelationComments, and GetConditionComments are never called anywhere in the codebase. These appear to be unused dead code that should be removed. The actual comment tracking is performed by storing comments directly in the listener's fields using GetCommentInfoForLine.
Suggested change
| // SetRelationComments sets comments for a relation within a type. | |
| func (ct *CommentTracker) SetRelationComments(typeName, relationName string, lineNum int) { | |
| commentInfo := ct.GetCommentInfoForLine(lineNum) | |
| if commentInfo == nil { | |
| return | |
| } | |
| if ct.typeComments[typeName] == nil { | |
| ct.typeComments[typeName] = &CommentsMetadata{} | |
| } | |
| if ct.typeComments[typeName].RelationComments == nil { | |
| ct.typeComments[typeName].RelationComments = make(map[string]*CommentInfo) | |
| } | |
| ct.typeComments[typeName].RelationComments[relationName] = commentInfo | |
| } | |
| // SetConditionComments sets comments for a condition. | |
| func (ct *CommentTracker) SetConditionComments(conditionName string, lineNum int) { | |
| commentInfo := ct.GetCommentInfoForLine(lineNum) | |
| if commentInfo == nil { | |
| return | |
| } | |
| ct.condComments[conditionName] = commentInfo | |
| } | |
| // GetTypeComments returns the comments metadata for a type. | |
| func (ct *CommentTracker) GetTypeComments(typeName string) *CommentsMetadata { | |
| return ct.typeComments[typeName] | |
| } | |
| // GetRelationComments returns comments for a relation. | |
| func (ct *CommentTracker) GetRelationComments(typeName, relationName string) *CommentInfo { | |
| if ct.typeComments[typeName] == nil { | |
| return nil | |
| } | |
| return ct.typeComments[typeName].RelationComments[relationName] | |
| } | |
| // GetConditionComments returns comments for a condition. | |
| func (ct *CommentTracker) GetConditionComments(conditionName string) *CommentInfo { | |
| return ct.condComments[conditionName] | |
| } |
Comment on lines
+65
to
+78
| // parseComments parses all comments from the source. | ||
| func (ct *CommentTracker) parseComments() { | ||
| for i, line := range ct.lines { | ||
| // Check for inline comment | ||
| if inlineIdx := strings.Index(line, " #"); inlineIdx != -1 { | ||
| // Make sure this isn't inside a condition expression or similar | ||
| // We only consider it an inline comment if there's actual code before it | ||
| beforeComment := strings.TrimSpace(line[:inlineIdx]) | ||
| if len(beforeComment) > 0 { | ||
| ct.lineComments[i] = strings.TrimSpace(line[inlineIdx+1:]) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The parseComments method is called in both NewCommentTracker and NewCommentTrackerWithMapping constructors, but it only populates the lineComments field which is never read anywhere in the codebase. This method can be removed along with the unused lineComments field.
Comment on lines
+29
to
+37
| // CommentTracker tracks comments in DSL source and maps them to AST elements. | ||
| type CommentTracker struct { | ||
| lines []string | ||
| lineComments map[int]string // line number -> inline comment | ||
| modelComment *ModelComments // comments before the model declaration | ||
| typeComments map[string]*CommentsMetadata // type name -> comments metadata | ||
| condComments map[string]*CommentInfo // condition name -> comment info | ||
| cleanedToOriginal map[int]int // mapping from cleaned line numbers to original | ||
| } |
There was a problem hiding this comment.
The Go CommentTracker has several unused fields and methods that should be removed for code cleanliness. The fields lineComments, modelComment, typeComments, and condComments in the struct are initialized but never actually used by the tracking logic. Similarly, the methods SetTypeComments, SetRelationComments, SetConditionComments, GetTypeComments, GetRelationComments, and GetConditionComments are defined but never called in the codebase. The actual comment tracking is done directly in the listener's fields, not in the CommentTracker. Consider removing these unused fields and methods to reduce code complexity and maintenance burden.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/go/transformer/dsltojson.go (1)
559-576: Comment stripping is space‑only and not quote‑aware.Using
strings.TrimLeft(line, " ")andstrings.Split(line, " #")ignores tabs and will truncate valid content containing" #"inside string literals (e.g., condition expressions). This can corrupt expressions or cause parse failures. Considerstrings.TrimLeftFunc(line, unicode.IsSpace)plus a small quote‑aware scanner to locate comment delimiters.
🤖 Fix all issues with AI agents
In `@pkg/go/transformer/jsontodsl.go`:
- Around line 947-974: The parseConditionWithComments function drops
CommentInfo.inline when emitting the condition header; update it to include
inline comments from the provided JSONCommentBlock by formatting comments.Inline
and inserting that formatted inline string into the header line (i.e., emit
"condition <name>(<params>)<inline> {"). Concretely, in
parseConditionWithComments check comments != nil and comments.Inline (or
equivalent) -> produce an inline comment string via the existing inline-format
helper (or add a small helper like formatInlineComment) and include that
variable in the fmt.Sprintf format so the header becomes "%scondition %s(%s)%s
{\n" and preserve the rest of the output.
In `@pkg/js/tests/comments.test.ts`:
- Around line 175-179: Change the string quote style for the ip_check expression
to use double quotes to satisfy ESLint: locate the ip_check object (symbol
"ip_check") and update the expression property (symbol "expression") so it uses
double quotes instead of single quotes (e.g., replace 'ip == "127.0.0.1"' with a
double-quoted outer string) to resolve the pipeline eslint violation.
In `@pkg/js/transformer/dsltojson.ts`:
- Around line 623-633: The inline-comment stripping in the loop over
originalLines uses line.split(" #") which only matches a space before '#' and
doesn’t respect quotes; replace that logic in the block that sets cleanedLine
(inside the for loop that iterates originalLines) with a quote-aware scanner
that scans the line char-by-char, tracks entering/exiting single and double
quotes (and escaped quotes), and treats a '#' as the start of a comment only if
the previous character is any whitespace (space, tab, etc.) and the scanner is
not currently inside a quoted string; take the substring up to that
comment-start index and trimEnd() for cleanedLine. If there is an existing
helper getInlineComment, reuse or refactor it to perform this same quoted-aware,
any-whitespace-before-# detection and call it from here instead of line.split("
#").
In `@pkg/js/transformer/jsontodsl.ts`:
- Around line 588-592: The thrown Error message with interpolation of
relationName and typeName is over 120 characters; refactor the long template
string used in the throw new Error(...) into a shorter expression to satisfy the
linter (for example, build the message in a local variable or split the string
across concatenated parts) so the resulting line lengths are under 120 chars;
locate the throw new Error that references relationName and typeName and replace
the long inline template literal with a composed message (const msg = ...; throw
new Error(msg)) or split the literal into multiple shorter strings joined
together.
- Line 613: The return statement constructing the DSL string in jsontodsl (which
uses conditionCommentsStr, conditionName, paramsString, conditionDef.expression,
and sourceString) is over 120 characters; split it into multiple shorter
concatenations or intermediate variables (e.g., build header =
`${conditionCommentsStr}condition ${conditionName}(${paramsString}) {\n`, body =
` ${conditionDef.expression}\n}`, then return
`${header}${body}${sourceString}\n`) so each line stays under 120 chars and the
final assembled string is unchanged.
Comment on lines
+947
to
+974
| // parseConditionWithComments parses a condition with comments. | ||
| func parseConditionWithComments(conditionName string, conditionDef *openfgav1.Condition, includeSourceInformation bool, comments *JSONCommentBlock) (string, error) { | ||
| if conditionName != conditionDef.GetName() { | ||
| return "", errors.ConditionNameDoesntMatchError(conditionName, conditionDef.GetName()) | ||
| } | ||
|
|
||
| // Build condition comment prefix | ||
| conditionCommentsStr := "" | ||
| if comments != nil { | ||
| conditionCommentsStr = formatCommentLines(comments.PrecedingLines) | ||
| } | ||
|
|
||
| paramsString := parseConditionParams(conditionDef.GetParameters()) | ||
| sourceString := constructSourceComment( | ||
| conditionDef.GetMetadata().GetModule(), | ||
| conditionDef.GetMetadata().GetSourceInfo().GetFile(), | ||
| "", includeSourceInformation, | ||
| ) | ||
|
|
||
| return fmt.Sprintf( | ||
| "%scondition %s(%s) {\n %s\n}%s\n", | ||
| conditionCommentsStr, | ||
| conditionDef.GetName(), | ||
| paramsString, | ||
| conditionDef.GetExpression(), | ||
| sourceString, | ||
| ), nil | ||
| } |
Contributor
There was a problem hiding this comment.
Condition inline comments are dropped on JSON→DSL.
CommentInfo.inline is captured on the DSL→JSON path, but parseConditionWithComments never emits inline comments. That loses data on round-trip. Add inline formatting to the condition header line.
💡 Proposed fix
- // Build condition comment prefix
- conditionCommentsStr := ""
- if comments != nil {
- conditionCommentsStr = formatCommentLines(comments.PrecedingLines)
- }
+ // Build condition comment prefix
+ conditionCommentsStr := ""
+ conditionInlineComment := ""
+ if comments != nil {
+ conditionCommentsStr = formatCommentLines(comments.PrecedingLines)
+ conditionInlineComment = formatInlineComment(comments.Inline)
+ }
@@
- return fmt.Sprintf(
- "%scondition %s(%s) {\n %s\n}%s\n",
- conditionCommentsStr,
- conditionDef.GetName(),
- paramsString,
- conditionDef.GetExpression(),
- sourceString,
- ), nil
+ return fmt.Sprintf(
+ "%scondition %s(%s) {%s\n %s\n}%s\n",
+ conditionCommentsStr,
+ conditionDef.GetName(),
+ paramsString,
+ conditionInlineComment,
+ conditionDef.GetExpression(),
+ sourceString,
+ ), nil🤖 Prompt for AI Agents
In `@pkg/go/transformer/jsontodsl.go` around lines 947 - 974, The
parseConditionWithComments function drops CommentInfo.inline when emitting the
condition header; update it to include inline comments from the provided
JSONCommentBlock by formatting comments.Inline and inserting that formatted
inline string into the header line (i.e., emit "condition
<name>(<params>)<inline> {"). Concretely, in parseConditionWithComments check
comments != nil and comments.Inline (or equivalent) -> produce an inline comment
string via the existing inline-format helper (or add a small helper like
formatInlineComment) and include that variable in the fmt.Sprintf format so the
header becomes "%scondition %s(%s)%s {\n" and preserve the rest of the output.
Comment on lines
+623
to
+633
| for (let originalIdx = 0; originalIdx < originalLines.length; originalIdx++) { | ||
| const line = originalLines[originalIdx]; | ||
| let cleanedLine = ""; | ||
|
|
||
| if (line.trimStart().length === 0) { | ||
| // Empty line | ||
| } else if (line.trimStart()[0] === "#") { | ||
| cleanedLine = ""; | ||
| } else { | ||
| cleanedLine = line.split(" #")[0].trimEnd(); | ||
| } |
Contributor
There was a problem hiding this comment.
Inline comment stripping isn’t quote‑aware and is space‑only.
line.split(" #") will truncate valid content like condition expressions containing " #" inside string literals, and inline comments after tabs won’t be removed, which can break parsing. Consider a small scanner that skips # inside quotes and treats any whitespace before # as a comment delimiter (and reuse it for getInlineComment).
🤖 Prompt for AI Agents
In `@pkg/js/transformer/dsltojson.ts` around lines 623 - 633, The inline-comment
stripping in the loop over originalLines uses line.split(" #") which only
matches a space before '#' and doesn’t respect quotes; replace that logic in the
block that sets cleanedLine (inside the for loop that iterates originalLines)
with a quote-aware scanner that scans the line char-by-char, tracks
entering/exiting single and double quotes (and escaped quotes), and treats a '#'
as the start of a comment only if the previous character is any whitespace
(space, tab, etc.) and the scanner is not currently inside a quoted string; take
the substring up to that comment-start index and trimEnd() for cleanedLine. If
there is an existing helper getInlineComment, reuse or refactor it to perform
this same quoted-aware, any-whitespace-before-# detection and call it from here
instead of line.split(" #").
pkg/js/transformer/jsontodsl.ts
Outdated
| const paramsString = parseConditionParams(conditionDef.parameters || {}); | ||
| const sourceString = constructSourceComment(conditionDef.metadata, "", includeSourceInformation); | ||
|
|
||
| return `${conditionCommentsStr}condition ${conditionName}(${paramsString}) {\n ${conditionDef.expression}\n}${sourceString}\n`; |
Contributor
There was a problem hiding this comment.
Line length exceeds 120 characters.
Line 613 exceeds the maximum allowed line length of 120 characters per the project's linting rules.
🔧 Suggested fix
- return `${conditionCommentsStr}condition ${conditionName}(${paramsString}) {\n ${conditionDef.expression}\n}${sourceString}\n`;
+ return `${conditionCommentsStr}condition ${conditionName}(${paramsString}) {\n` +
+ ` ${conditionDef.expression}\n}${sourceString}\n`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return `${conditionCommentsStr}condition ${conditionName}(${paramsString}) {\n ${conditionDef.expression}\n}${sourceString}\n`; | |
| return `${conditionCommentsStr}condition ${conditionName}(${paramsString}) {\n` + | |
| ` ${conditionDef.expression}\n}${sourceString}\n`; |
🧰 Tools
🪛 GitHub Check: test / lint
[warning] 613-613:
This line has a length of 130. Maximum allowed is 120
🤖 Prompt for AI Agents
In `@pkg/js/transformer/jsontodsl.ts` at line 613, The return statement
constructing the DSL string in jsontodsl (which uses conditionCommentsStr,
conditionName, paramsString, conditionDef.expression, and sourceString) is over
120 characters; split it into multiple shorter concatenations or intermediate
variables (e.g., build header = `${conditionCommentsStr}condition
${conditionName}(${paramsString}) {\n`, body = `
${conditionDef.expression}\n}`, then return `${header}${body}${sourceString}\n`)
so each line stays under 120 chars and the final assembled string is unchanged.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Add bidirectional comment preservation between DSL and JSON formats, enabling comments to survive round-trip transformations (DSL → JSON → DSL).
What problem is being solved?
Currently, when converting OpenFGA DSL models to JSON format, all comments are stripped and lost. This makes it difficult to maintain documentation and context when models are transformed between formats. Users who add helpful comments to their DSL models lose that information when the model goes through any JSON transformation.
How is it being solved?
Comments are now captured during DSL parsing and stored in optional metadata fields within the JSON structure. When converting JSON back to DSL, these comments are emitted in their original positions. The implementation:
What changes are made to solve it?
New files:
Modified files:
JSON schema additions (all optional for backward compatibility):
Supported comment types:
References
Review Checklist
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.