Repository navigation
Conversation
XmlSchemaVisitor ignored isOptional(), so every optional field and relationship was written as a plain xs:element. XSD defaults minOccurs to 1, which made optional properties required: an instance leaving out an optional property failed validation. - Write minOccurs="0" for optional fields and relationships, and on the wrapper element of an optional map field with a size validator. - Optional arrays always get minOccurs="0", even when their size validator has a minimum, because an absent array is valid. - Pass the property's optional modifier into visitField for scalar fields. The field returned by getScalarField() does not carry it. - Move the occurrence attribute logic shared by fields and relationships into a toOccurs helper. Closes accordproject#304 Signed-off-by: NopeCharon <115794254+NopeCharon@users.noreply.github.com>
|
Thanks for your contribution, and for opening #304 with the details alongside this PR. The CI workflows have been approved and are passing. The PR now waits for review from the Concerto maintainers. If you push more changes, the checks will run again, and it helps if the commits are signed off (DCO). Drafted by an Accord Project agent, reviewed by a maintainer. |
![mira-accord-project[bot]](https://avatars.githubusercontent.com/in/5553948?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for this fix, and for a first contribution that is so carefully done. Writing minOccurs="0" for optional properties makes the generated schema match what Concerto allows. The new toOccurs helper puts the rule for fields, relationships and map fields in one place. Passing the property's isOptional() through for scalar-typed properties is a good catch, because the scalar field doesn't carry that modifier. Treating optional arrays as minOccurs="0" regardless of a declared minimum size is a sensible choice, since XSD can't express "absent, or at least n". The new unit tests cover each of these branches, and the snapshot changes all follow from the fix. I found nothing that blocks merging. One small coordination point is below. Once a maintainer has reviewed it and the ordering with the other open XML Schema change is settled, this looks ready to go.
Nothing blocking; some suggestions below. Findings: 1 nit.
Automated review by Mira, an Accord Project agent, at faab461. Findings are labelled blocking, should-fix or nit; a maintainer makes the final call.
| } | ||
| const mapName = field.getType(); | ||
| parameters.fileWriter.writeLine(2, `<xs:element name="${this.toXsName(field.getName())}">`); | ||
| parameters.fileWriter.writeLine(2, `<xs:element name="${this.toXsName(field.getName())}"${this.toOccurs(field, isOptional)}>`); |
There was a problem hiding this comment.
nit · hygiene
Pull request #308 also edits the map branch of visitField: it changes the const mapName line directly above this <xs:element name=...> line, and edits the same visitField map tests. Whichever of the two merges second will probably hit a merge conflict. There is nothing to change in this PR's logic. It would help to agree an order with #308 and rebase the other one afterwards, re-running npm run test:updateSnapshots if the snapshot needs regenerating.
Closes #304
Generated XML Schema now lets an instance leave out optional properties. Before this change, every optional field and relationship was written as a required element.
Changes
minOccurs="0"for optional fields and relationships, and on the wrapper element of an optional map field that has a size validator.visitFieldfor scalar fields, because the field returned bygetScalarField()does not carry it.minOccurs/maxOccurslogic shared by fields and relationships into atoOccurshelper.hrsnapshot (state, the map fields ofCompany,middleNames,manager).Flags
o String[] tags size=[2,5] optional) now getsminOccurs="0". A repeated element can't tell an absent array apart from an empty one, and an absent optional array has to validate, so the minimum can't be enforced in this case.visitField, a few lines from this change. Whichever PR is merged second will need a small rebase.xmllint --schema: an instance without its optional properties fails onmain(Missing child element(s). Expected is ( {org.acme}nick )) and validates with this change.Screenshots or Video
N/A
Related Issues
Author Checklist
--signoffoption of git commit.mainfromfork:branchname