Fix: Support @constraint:Date annotation mapping in OpenAPI specification#1886
Closed
sathindudezoysa wants to merge 4 commits into
Closed
Conversation
sathindudezoysa
requested review from
TharmiganK,
daneshk,
lnash94 and
shafreenAnfar
as code owners
📝 WalkthroughWalkthroughThis PR adds support for the Changes
Sequence Diagram(s)sequenceDiagram
participant AST as Ballerina AST
participant Mapper as ConstraintMapperImpl
participant Annotation as ConstraintAnnotation
participant Schema as Schema Generator
AST->>Mapper: `@constraint`:Date found
Mapper->>Mapper: extractFieldValue("date")
Mapper->>Annotation: withDate("date")
Annotation->>Annotation: store date field
Mapper->>Schema: constraintAnnot.getDate()
alt Date constraint present
Schema->>Schema: set StringSchema format="date"
end
Schema->>Schema: return configured schema
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ballerina-to-openapi/src/main/java/io/ballerina/openapi/service/mapper/constraint/ConstraintMapperImpl.java (1)
379-384: Remove the orphaned diagnostic enumOAS_CONVERTOR_120.The Date constraint is now properly handled and this diagnostic is no longer emitted. The enum value defined in
DiagnosticMessages.javais unused and can be removed as cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ballerina-to-openapi/src/main/java/io/ballerina/openapi/service/mapper/constraint/ConstraintMapperImpl.java` around lines 379 - 384, Remove the unused diagnostic enum OAS_CONVERTOR_120 from DiagnosticMessages.java and any related references now that isDateConstraint in ConstraintMapperImpl handles Date constraints; search for the symbol OAS_CONVERTOR_120 and delete its enum entry from the DiagnosticMessages enum (and remove any import/usages if found), ensuring no compilation errors remain and tests still pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@openapi-cli/src/test/java/io/ballerina/openapi/generators/openapi/ConstraintTests.java`:
- Around line 124-128: The test testDateConstraintMapping references
"constraint/expectedDate.yaml" but the added file is named expectedDate.yml;
rename the expected file to use the .yaml extension (matching the other
constraint tests) so TestUtils.compareWithGeneratedFile(ballerinaFilePath,
"constraint/expectedDate.yaml") finds it; ensure the renamed file preserves
contents and casing exactly (expectedDate.yaml) and update any CI/test artifacts
if they reference the .yml name.
---
Nitpick comments:
In
`@ballerina-to-openapi/src/main/java/io/ballerina/openapi/service/mapper/constraint/ConstraintMapperImpl.java`:
- Around line 379-384: Remove the unused diagnostic enum OAS_CONVERTOR_120 from
DiagnosticMessages.java and any related references now that isDateConstraint in
ConstraintMapperImpl handles Date constraints; search for the symbol
OAS_CONVERTOR_120 and delete its enum entry from the DiagnosticMessages enum
(and remove any import/usages if found), ensuring no compilation errors remain
and tests still pass.
🪄 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: c85d90da-9cd8-4240-b93d-b8ed3f68d797
📒 Files selected for processing (8)
ballerina-to-openapi/src/main/java/io/ballerina/openapi/service/mapper/constraint/ConstraintAnnotation.javaballerina-to-openapi/src/main/java/io/ballerina/openapi/service/mapper/constraint/ConstraintMapperImpl.javaopenapi-cli/src/test/java/io/ballerina/openapi/generators/openapi/ConstraintTests.javaopenapi-cli/src/test/resources/ballerina-to-openapi/constraint/date_constraint.balopenapi-cli/src/test/resources/ballerina-to-openapi/expected_gen/constraint/expectedDate.ymlopenapi-tool/BalTool.tomlopenapi-tool/Ballerina.tomlopenapi-tool/Dependencies.toml
Contributor
Author
|
Need to do some changes in the code |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ballerina-to-openapi/src/main/java/io/ballerina/openapi/service/mapper/constraint/ConstraintMapperImpl.java`:
- Around line 351-355: The code currently converts string/identifier annotation
values for every constraint field, causing non-date annotations (e.g.,
maxLength: MAX_LEN) to be coerced into strings and then fail numeric parsing; to
fix, after detecting a date constraint with isDateConstraint(annotation) and
calling constraintBuilder.withDate("date"), immediately short-circuit
(return/continue) to avoid passing this annotation to the generic extractor (the
MappingConstructorExpressionNode handling and extractFieldValue calls), leaving
extractFieldValue strict for numeric/other types; apply the same short-circuit
change to the other occurrence around lines 395-402 where date handling is
present.
🪄 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: d4a27854-4178-4397-b389-3e6e66b0adf5
📒 Files selected for processing (5)
ballerina-to-openapi/src/main/java/io/ballerina/openapi/service/mapper/constraint/ConstraintMapperImpl.javaballerina-to-openapi/src/main/java/io/ballerina/openapi/service/mapper/type/ReferenceTypeMapper.javaopenapi-cli/src/test/java/io/ballerina/openapi/generators/openapi/ConstraintTests.javaopenapi-cli/src/test/resources/ballerina-to-openapi/constraint/dateConstraint.balopenapi-cli/src/test/resources/ballerina-to-openapi/expected_gen/constraint/expectedDate.yaml
✅ Files skipped from review due to trivial changes (1)
- openapi-cli/src/test/resources/ballerina-to-openapi/constraint/dateConstraint.bal
🚧 Files skipped from review as they are similar to previous changes (1)
- openapi-cli/src/test/java/io/ballerina/openapi/generators/openapi/ConstraintTests.java
Comment on lines
351
to
355
| if (isDateConstraint(annotation)) { | ||
| ExceptionDiagnostic error = new ExceptionDiagnostic(DiagnosticMessages.OAS_CONVERTOR_120, | ||
| annotation.location(), annotation.toString()); | ||
| diagnostics.add(error); | ||
| return; | ||
| constraintBuilder.withDate("date"); | ||
| } | ||
|
|
||
| MappingConstructorExpressionNode annotationValue = annotation.annotValue().orElse(null); |
There was a problem hiding this comment.
Keep the date workaround out of the generic value extractor.
STRING_LITERAL and identifier expressions are now accepted for every constraint field. That means a non-date annotation like maxLength: MAX_LEN can flow through as "MAX_LEN" and hit the later Integer.valueOf(...) / parseInt(...) calls, which aborts conversion with NumberFormatException instead of emitting the existing diagnostic. If the goal is just to tolerate option / message inside @constraint:Date, short-circuit that branch after withDate("date") and leave extractFieldValue() strict.
Proposed fix
.forEach(annotation -> {
if (isDateConstraint(annotation)) {
constraintBuilder.withDate("date");
+ return;
}
MappingConstructorExpressionNode annotationValue = annotation.annotValue().orElse(null);
if (Objects.isNull(annotationValue)) {
return;
@@
switch (syntaxKind) {
case NUMERIC_LITERAL:
- case STRING_LITERAL:
- case QUALIFIED_NAME_REFERENCE:
- case SIMPLE_NAME_REFERENCE:
return Optional.of(exprNode.toString().trim());Also applies to: 395-402
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@ballerina-to-openapi/src/main/java/io/ballerina/openapi/service/mapper/constraint/ConstraintMapperImpl.java`
around lines 351 - 355, The code currently converts string/identifier annotation
values for every constraint field, causing non-date annotations (e.g.,
maxLength: MAX_LEN) to be coerced into strings and then fail numeric parsing; to
fix, after detecting a date constraint with isDateConstraint(annotation) and
calling constraintBuilder.withDate("date"), immediately short-circuit
(return/continue) to avoid passing this annotation to the generic extractor (the
MappingConstructorExpressionNode handling and extractFieldValue calls), leaving
extractFieldValue strict for numeric/other types; apply the same short-circuit
change to the other occurrence around lines 395-402 where date handling is
present.
Contributor
Author
|
closed this PR with the aid of #1887 |
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.
Purpose
Currently, Ballerina's
@constraint:Dateconstraints are not accurately mapped into the OpenAPI specification. The constraint mapper was failing to detect theDateconstraint due to brittle string-matching logic and prematurely aborted processing if it encountered unsupported nested fields (likeoption: constraint:PASTormessage) or an empty mapping constructor.Resolves #5049
Goals
This fix ensures that any Ballerina record field annotated with
@constraint:Dateis accurately reflected in the generated OpenAPI schema withtype: stringandformat: date. It also ensures that the mapper does not fail or skip processing when Ballerina-specific constraint options (likePAST,FUTURE, or custom messages) are present in the annotation.Approach
The solution was implemented by modifying
ConstraintMapperImpl.javaandConstraintAnnotation.java:dateproperty to theConstraintAnnotationmodel and its corresponding builder. This allows the mapper to track the presence of a Date constraint in memory without requiring standard constraint boundaries likeminValueormaxLength.extractedConstraintAnnotationto set thedateflag and immediatelyreturnwhen the@constraint:Dateannotation is detected. This early exit safely ignores unsupported Ballerina-specific Date constraint fields (likeoption: constraint:PASTand custommessages) by preventing the AST parser from diving into them, completely avoiding the diagnostic conversion errors that previously crashed the generator.setStringConstraintValuesToSchemamethod to check for the presence of thedateconstraint flag. If present, it explicitly appliesproperties.setFormat("date")to the resulting OpenAPI string schema.User stories
As an API developer using Ballerina, I want my
@constraint:Dateannotations to automatically generate theformat: dateproperty in my OpenAPI specification, so that API consumers and generated client SDKs correctly interpret the field as a Date.Release note
Fix: Support
@constraint:Dateannotation mapping to OpenAPI specification.Automation tests
testDateConstraintMappingto verify that a Ballerina record with a@constraint:Datefield correctly maps to an OpenAPI schema withtype: stringandformat: date. Validated againstdate_constraint.balandexpectedDate.yaml.Security checks
Test environment
Learning
Identified that utilizing Ballerina's AST (
QualifiedNameReferenceNode) for syntax tree traversal is far more resilient to code formatting variations than performing raw string comparisons on node text representations.Summary
This pull request enhances the OpenAPI specification generation to properly support Ballerina
@constraint:Dateannotations. Previously, the constraint mapper would fail to process Date constraints due to issues handling Ballerina-specific nested constraint fields and custom message attributes.Changes
Core Mapping Logic:
ConstraintAnnotationmodel to track Date constraints via a newdateproperty, including builder support and accessor methods@constraint:Dateannotations while avoiding traversal of unsupported nested fieldsType Handling:
time:Datereference types to automatically format them as OpenAPI date stringsTesting and Dependencies:
testDateConstraintMapping) validating the mapping of constrained date fields to OpenAPI schemas with correct type and format attributesOutcome
These changes enable the OpenAPI tools to correctly generate API specifications for Ballerina services that use date constraints, improving compatibility and specification accuracy for time-based validation requirements.