Repository navigation
COMMS-5397 OpenAPI-ready @Schema annotations for the v3.0 model - #87
Draft
cryptalith wants to merge 7 commits into
Draft
cryptalith wants to merge 7 commits into
cryptalith wants to merge 7 commits into
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
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.
What
Annotation-only changes, so the Registry can generate an OpenAPI 3 description of API v3.0 from this model (COMMS-5397). Nothing changes in the classes, fields, enum constants, JAXB/Jackson output or dependencies. The version is bumped to 4.0.3.
Why
swagger-core reads
io.swagger.v3.oas.annotations.media.Schema. The 2023 conversion, and one later edit, left four problems behind:description(for example"WorkV3_0") instead ofname. Schemas were therefore named after XML elements, andrecord.Worksandrecord.summary.Worksoverwrote each other, as didEducations,EmploymentsandCreditName.allowableValueswas one comma-joined string, so every enum came out as a single value. A missing comma inWork.type, from the commit that added new work types (ac6e8de, 2024), also producedclinical-studyconference-abstract.funded-bywas missing from relationship, andprivatefrom visibility.JOURNAL_ARTICLE,SALARY_AWARD,AR, …) instead of the values that go over the wire. This affectsWorkSummary.type,FundingSummary.type,PeerReviewSummaryrole and type,Preferences.locale,History.creationMethod, and nine visibility fields (Biography, City, Email, Name and the five activity summaries).Changes
Schema names. Every JAXB-bound v3.0 class gets
@Schema(name = …): the old label withoutV3_0, or the simple class name for the eight classes that had no label (ClientSummary,GroupIdRecord,GroupIdRecords,NotificationAmended,NotificationCustom,TransientError,TransientNonEmptyString, andrecord.CreditName).WorkSource(its label was justV3_0),ExpandedSearch(its label duplicatedSearch), andRecordCreditName(unlabelled, and its simple name clashes withcommon.CreditName).CreationMethodV2_0frommessage.CreationMethod, whichHistory.creationMethoduses.Wire values. Every enum-valued field documents its JSON wire values as an array: what the field's
XmlAdapterwrites, narrowed by the XSD enumeration where the XSD lists a subset (visibility dropssystem). Where JSON and the XSD disagree, JSON wins:creation-methoddocumentsAPI,DIRECT,MEMBER_REFERRED,WEBSITE,INTEGRATION_TEST, not the XSD'sDirectorwebsite.Contributor roles. Both
contributor-rolefields areStrings. They now list the values reads return and writes accept, read from the API's own 9058 error in a local test stack (writes also take a few aliases the API normalises, such asCONCEPTUALIZATION):lead,co-lead,supported-by,other-contribution, plus the 14 CRediT role URIs.Before, both listed the 11 work roles, so funding documented values the API refuses and neither had the CRediT URIs the API accepts and returns.
Guard test.
OpenApiSchemaAnnotationsTestenforces all of the above. It checks that names are unique; that noXxxV3_0orXxxV2_0label remains on a model class or on an enum a v3 field uses; that each enum field's values equal its JSON values (a field needs no annotation when its constants already match); and that the two contributor-role lists equal the role enums plus the CRediT URIs incommon-3.0.xsd.main(9592e2d) the test fails with 146 + 143 + 58 + 2 problems (names, labels, enum fields, contributor roles); here it passes.Required properties
Later commits make
requiredmatch what the API does. The COMMS-5397 contract tests (a private ORCID repo)remove each top-level property of the create body of each of the 16 record sections in turn; before these
commits, 15 of the 16 disagreed with the API. A second review checked the nested properties against the XSDs,
which the Registry validates every write against, and the API.
@Schema(required = true)where the API refuses a body without the property:titleandtype(the XSD validation refuses the body, 9047),external-ids, which needs at least oneselfidentifier on create and update (9023);external-ids;reviewer-role,review-identifiers,review-type,review-completion-date,convening-organization;proposal,resource-item;content;url;url,relationship(9020);notification-subject,notification-intro(9047).Funding and research-resource proposal
start-dateare required only with anend-date(9050), and anorganization address's
cityandcountryonly for education and employment (9060); these get descriptionsinstead.
Ten
@XmlElement(required = true)flags removed, where this repo's own XSDs make the element optional and theAPI accepts a body without it (each checked against the running API):
Affiliation.startDate(common-3.0.xsdminOccurs="0");AuthorizationUrluri,pathandhost: notification-permission-3.0.xsd takes uri (with or withoutpath) or path alone, with an optional host;
ContributorAttributescontributor-sequenceandcontributor-role, andFundingContributorAttributescontributor-role(work-3.0.xsd, funding-3.0.xsd);Item'sexternal-id(notification-permission-3.0.xsd);OrganizationAddresscityandcountry(common-3.0.xsd).TransientError's two flags also disagree with the XSD; the type only appears in responses, where bothfields are always written, so they are left alone.
These are the only non-
@Schemalines in the PR.@Schemacannot undo a JAXBrequiredat theswagger-annotations version this repo compiles against (2.1.1 has no
requiredMode); swagger-core andJackson's JAXB introspector both read the flag. Marshalling and unmarshalling are unchanged:
required, and Jackson enforces it only on creator properties, which these classes lack;Verification
mvn testruns 192 tests with 0 failures across 18 classes, including the guard test.Every changed line under
src/mainis a@Schemaannotation or its import, apart from the ten removedrequired = trueflags above.Spike output from swagger-core-jakarta 2.2.55:
For nested references to use these names, the Registry's generator has to stop Jackson's JAXB introspector from supplying XML root names. That change lands in ORCID-Source, not here.
Notes for review
Item'sitem-typekeeps all 13 types, becauseItemsis shared with amended notifications, and its description lists the ten a permission notification accepts (9047).PersonExternalIdentifier.relationshipkeeps all four relationship values, because a response carries whatever is stored, and its description says onlyselfis accepted on writes (9020).Notification.notificationTypelists all eight types for every notification class: the field is shared, and narrowing it forNotificationPermission(whose XSD allows onlypermission) would need a field override, not an annotation.service-anouncement.Notification.notificationTypedocuments this spelling because that is the value on the wire.mvn packagefails inmaven-javadoc-pluginon existing references ingroupid_v2/GroupIdRecords.javaandv3/release/groupid/GroupIdRecords.java.mainfails the same way; this change doesn't touch it.