Set sequenceGroup optional when all child elements are optional#39
Closed
Nuvindu wants to merge 4 commits intoballerina-platform:mainfrom
Closed
Set sequenceGroup optional when all child elements are optional#39Nuvindu wants to merge 4 commits intoballerina-platform:mainfrom
Nuvindu wants to merge 4 commits intoballerina-platform:mainfrom
Conversation
Nuvindu
requested review from
NipunaRanasinghe and
shafreenAnfar
as code owners
📝 WalkthroughWalkthroughThe pull request introduces logic to mark XSD sequences as optional when all their child elements are optional. A new helper method determines if all child elements have minOccurs=0, and this flag is passed to the sequence annotation method to conditionally append a question mark to sequence names in generated Ballerina code. Changes
Sequence Diagram(s)Sequence diagram generation does not apply to these changes. While the implementation introduces new control flow for determining sequence optionality, the changes primarily affect code generation logic within a single visitor method rather than introducing interactions between multiple distinct system components with a clear sequential flow that would benefit from visualization. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xsd-core/src/main/java/io/ballerina/xsd/core/visitor/XSDVisitorImpl.java (1)
749-769:⚠️ Potential issue | 🟠 MajorInclude sequence
minOccurs=0in the optionality decision.Right now optionality is driven only by
allChildrenOptional. A sequence withminOccurs=0but required children should also render the sequence group as optional; this currently stays required.🔧 Suggested fix
builder.append(WHITESPACE).append(convertToCamelCase(sequenceName)); - if (allChildrenOptional) { + boolean sequenceOptional = allChildrenOptional || ZERO.equals(minOccurrence); + if (sequenceOptional) { builder.append(QUESTION_MARK); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xsd-core/src/main/java/io/ballerina/xsd/core/visitor/XSDVisitorImpl.java` around lines 749 - 769, In applySequenceAnnotation, optionality is only decided by allChildrenOptional so sequences with minOccurs="0" remain required; update the optionality check that appends QUESTION_MARK to consider minOccurrence as well (e.g., treat minOccurrence == "0" as optional) by changing the condition that currently uses allChildrenOptional to (allChildrenOptional || "0".equals(minOccurrence)) so sequences with minOccurs=0 render as optional; refer to applySequenceAnnotation, minOccurrence, ONE, and the builder.append(QUESTION_MARK) call to locate the change.
🧹 Nitpick comments (1)
xsd-core/src/test/java/io/ballerina/xsd/core/XSDToRecordTest.java (1)
31-33: Avoid repo-root–dependent resource paths.Hard-coding
xsd-core/src/test/resourcesmakes the test sensitive to the working directory. Consider resolving from the test classpath to avoid failures when running tests from the module directory or IDE.🔧 Suggested refactor
import java.nio.file.Paths; +import java.util.Objects; import java.util.stream.Stream; public class XSDToRecordTest { - private static final Path RES_DIR = Paths.get("xsd-core/src/test/resources/").toAbsolutePath(); + private static final Path RES_DIR = Paths.get( + Objects.requireNonNull(XSDToRecordTest.class.getClassLoader().getResource("")).getPath());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xsd-core/src/test/java/io/ballerina/xsd/core/XSDToRecordTest.java` around lines 31 - 33, The test currently hard-codes RES_DIR = Paths.get("xsd-core/src/test/resources").toAbsolutePath() which breaks when tests are run from different working directories; change the code in XSDToRecordTest to resolve test resources from the classpath instead (use XSDToRecordTest.class.getResource(...) or Thread.currentThread().getContextClassLoader().getResource/getResourceAsStream) to locate the "xml" resource directory or individual files rather than building a repo-root path, and update usages of RES_DIR and XML_DIR to use the URL/Path returned by the classloader (or use getResourceAsStream for file reads) so tests work regardless of working directory or IDE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xsd-core/src/main/java/io/ballerina/xsd/core/visitor/XSDVisitorImpl.java`:
- Around line 773-788: areAllChildrenOptional currently only inspects children
whose localName equals ELEMENT, so required particles like CHOICE/SEQUENCE/ANY
are ignored; update areAllChildrenOptional to treat other particle node types
(e.g., CHOICE, SEQUENCE, ANY) the same way as ELEMENT: for any child whose
localName is one of ELEMENT, CHOICE, SEQUENCE, ANY (use the existing ELEMENT,
MIN_OCCURS, ZERO constants and add CHOICE/SEQUENCE/ANY constants if needed),
check its minOccurs attribute and return false if minOccurs is absent or not
"0"; also mark that a particle was seen (replace hasElement with hasParticle or
set hasElement when any of these particle types are encountered) so the method
only returns true when at least one particle was inspected.
---
Outside diff comments:
In `@xsd-core/src/main/java/io/ballerina/xsd/core/visitor/XSDVisitorImpl.java`:
- Around line 749-769: In applySequenceAnnotation, optionality is only decided
by allChildrenOptional so sequences with minOccurs="0" remain required; update
the optionality check that appends QUESTION_MARK to consider minOccurrence as
well (e.g., treat minOccurrence == "0" as optional) by changing the condition
that currently uses allChildrenOptional to (allChildrenOptional ||
"0".equals(minOccurrence)) so sequences with minOccurs=0 render as optional;
refer to applySequenceAnnotation, minOccurrence, ONE, and the
builder.append(QUESTION_MARK) call to locate the change.
---
Nitpick comments:
In `@xsd-core/src/test/java/io/ballerina/xsd/core/XSDToRecordTest.java`:
- Around line 31-33: The test currently hard-codes RES_DIR =
Paths.get("xsd-core/src/test/resources").toAbsolutePath() which breaks when
tests are run from different working directories; change the code in
XSDToRecordTest to resolve test resources from the classpath instead (use
XSDToRecordTest.class.getResource(...) or
Thread.currentThread().getContextClassLoader().getResource/getResourceAsStream)
to locate the "xml" resource directory or individual files rather than building
a repo-root path, and update usages of RES_DIR and XML_DIR to use the URL/Path
returned by the classloader (or use getResourceAsStream for file reads) so tests
work regardless of working directory or IDE.
Comment on lines
+773
to
+788
| private boolean areAllChildrenOptional(NodeList childNodes) { | ||
| boolean hasElement = false; | ||
| for (Node child : asIterable(childNodes)) { | ||
| if (child.getNodeType() != Node.ELEMENT_NODE) { | ||
| continue; | ||
| } | ||
| if (!ELEMENT.equals(child.getLocalName())) { | ||
| continue; | ||
| } | ||
| hasElement = true; | ||
| Node minOccursAttr = child.getAttributes().getNamedItem(MIN_OCCURS); | ||
| if (minOccursAttr == null || !ZERO.equals(minOccursAttr.getNodeValue())) { | ||
| return false; | ||
| } | ||
| } | ||
| return hasElement; |
There was a problem hiding this comment.
Optionality check ignores non-element particles.
areAllChildrenOptional only looks at <element> nodes. If a sequence contains a required <choice>, <sequence>, or <any>, the method can still return true (when element children are optional), causing an incorrect optional sequence group.
🔧 Suggested fix
- for (Node child : asIterable(childNodes)) {
- if (child.getNodeType() != Node.ELEMENT_NODE) {
- continue;
- }
- if (!ELEMENT.equals(child.getLocalName())) {
- continue;
- }
+ for (Node child : asIterable(childNodes)) {
+ if (child.getNodeType() != Node.ELEMENT_NODE
+ || ANNOTATION.equals(child.getLocalName())) {
+ continue;
+ }
hasElement = true;
Node minOccursAttr = child.getAttributes().getNamedItem(MIN_OCCURS);
if (minOccursAttr == null || !ZERO.equals(minOccursAttr.getNodeValue())) {
return false;
}
}📝 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
| private boolean areAllChildrenOptional(NodeList childNodes) { | |
| boolean hasElement = false; | |
| for (Node child : asIterable(childNodes)) { | |
| if (child.getNodeType() != Node.ELEMENT_NODE) { | |
| continue; | |
| } | |
| if (!ELEMENT.equals(child.getLocalName())) { | |
| continue; | |
| } | |
| hasElement = true; | |
| Node minOccursAttr = child.getAttributes().getNamedItem(MIN_OCCURS); | |
| if (minOccursAttr == null || !ZERO.equals(minOccursAttr.getNodeValue())) { | |
| return false; | |
| } | |
| } | |
| return hasElement; | |
| private boolean areAllChildrenOptional(NodeList childNodes) { | |
| boolean hasElement = false; | |
| for (Node child : asIterable(childNodes)) { | |
| if (child.getNodeType() != Node.ELEMENT_NODE | |
| || ANNOTATION.equals(child.getLocalName())) { | |
| continue; | |
| } | |
| hasElement = true; | |
| Node minOccursAttr = child.getAttributes().getNamedItem(MIN_OCCURS); | |
| if (minOccursAttr == null || !ZERO.equals(minOccursAttr.getNodeValue())) { | |
| return false; | |
| } | |
| } | |
| return hasElement; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@xsd-core/src/main/java/io/ballerina/xsd/core/visitor/XSDVisitorImpl.java`
around lines 773 - 788, areAllChildrenOptional currently only inspects children
whose localName equals ELEMENT, so required particles like CHOICE/SEQUENCE/ANY
are ignored; update areAllChildrenOptional to treat other particle node types
(e.g., CHOICE, SEQUENCE, ANY) the same way as ELEMENT: for any child whose
localName is one of ELEMENT, CHOICE, SEQUENCE, ANY (use the existing ELEMENT,
MIN_OCCURS, ZERO constants and add CHOICE/SEQUENCE/ANY constants if needed),
check its minOccurs attribute and return false if minOccurs is absent or not
"0"; also mark that a particle was seen (replace hasElement with hasParticle or
set hasElement when any of these particle types are encountered) so the method
only returns true when at least one particle was inspected.
Contributor
Author
|
Closing this as these changes are already applied by #40 |
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
Summary
This pull request introduces logic to automatically mark sequence groups as optional when all of their child elements are optional, improving the accuracy of generated XML data binding code.
Changes
Core Implementation:
XSDVisitorImpl.javato compute whether all child elements in a sequence are optional and conditionally append a question mark to the sequence group name when this condition is trueminOccursset to 0applySequenceAnnotationmethod signature to accept a boolean flag indicating whether all children are optionalTest Coverage:
45_sequence_with_all_optional_elements.xsdand corresponding expected outputTest Expectations:
?suffix)Impact
This change ensures that generated Ballerina record types more accurately reflect the optionality constraints defined in the source XSD schemas, reducing the need for manual adjustments to the generated code and improving data binding semantics.