Update spec according to agreement in Sprint Planning#619
Open
Update spec according to agreement in Sprint Planning#619
Conversation
Review Summary by QodoMerge location codes into unified locations object structure
WalkthroughsDescription• Consolidate UNLocationCodes and facilitySMDGCodes into unified locations object • Change location items from strings to objects supporting both code types • Allow optional UNLocationCode and/or facilitySMDGCode in each location • Update documentation to reflect new combined location structure Diagramflowchart LR
A["UNLocationCodes array<br/>string items"] --> B["locations array<br/>object items"]
C["facilitySMDGCodes array<br/>string items"] --> B
B --> D["UNLocationCode property<br/>optional string"]
B --> E["facilitySMDGCode property<br/>optional string"]
File Changes1. ovs_hub_ntf/v1/OVS_HUB_NTF_v1.0.0.yaml
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Updates the OVS Hub Notification (NTF) OpenAPI spec to align subscription location filtering with the sprint-planning agreement by allowing combined UN/LOCODE + facility SMDG code filtering.
Changes:
- Replaced separate
UNLocationCodesandfacilitySMDGCodesfilters with a singlelocationsarray of objects. - Defined
locations[]objects with optionalUNLocationCodeandfacilitySMDGCodefields to allow combinations.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| An array of **UNLocationCode and/or facilitySMDGCode combinations** to match with this subscription filter. If the array consists of more than one item - a logical **OR** is used between the objects when matching. | ||
|
|
||
| If this property is empty, you will be notified for all `UNLocationCodes` that have changes you are authorized to see. | ||
| If this property is empty, you will be notified for all `locations` that have changes you are authorized to see. |
There was a problem hiding this comment.
locations sets minItems: 1 but the description says that an empty property results in notifications for all locations. With minItems: 1 an empty array is invalid, so either remove minItems (to allow []) or update the wording to clarify that omitting the property (not an empty array) means ‘all’.
Suggested change
| If this property is empty, you will be notified for all `locations` that have changes you are authorized to see. | |
| If this property is omitted, you will be notified for all `locations` that have changes you are authorized to see. |
Comment on lines
+797
to
+801
| It is possible to provide the `UNLocationCode`, the `facilitySMDGCode` or both when specifying the location. | ||
|
|
||
| - 2 characters for the country code using [ISO 3166-1 alpha-2](https://www.iso.org/obp/ui/#iso:pub:PUB500001:en) | ||
| - 3 characters to code a location within that country. Letters A-Z and numbers from 2-9 can be used | ||
| If multiple `UNLocationCode` or `facilitySMDGCode` values need to be provided, then they must be specified as separate objects in the `locations` array. | ||
|
|
||
| More info can be found here: [UN/LOCODE](https://unece.org/trade/cefact/UNLOCODE-Download) | ||
| example: NLAMS | ||
| facilitySMDGCodes: | ||
| type: array | ||
| description: | | ||
| An array of **Facility SMDG codes** to match with this subscription filter. If the array consists of more than one item - a logical **OR** is used between the values when matching. | ||
| **Condition:** At least one of `UNLocationCode` and/or `facilitySMDGCode` **MUST** be specified, both are also allowed. |
There was a problem hiding this comment.
The locations item schema is an object with no validation to ensure at least one of UNLocationCode or facilitySMDGCode is present, so {} currently satisfies the schema despite the stated condition. Consider encoding the condition with anyOf/oneOf + required (or otherwise enforcing at least one property) to prevent empty objects.
Comment on lines
+802
to
+808
| properties: | ||
| UNLocationCode: | ||
| type: string | ||
| pattern: ^[A-Z]{2}[A-Z2-9]{3}$ | ||
| minLength: 5 | ||
| maxLength: 5 | ||
| description: | |
There was a problem hiding this comment.
The locations filter item repeats the same UNLocationCode/facilitySMDGCode structure already defined in #/components/schemas/Location (and again in SubscriptionBodyWithSecret). To reduce duplication and keep the two request/response schemas aligned, consider referencing a shared schema (e.g., $ref to Location or a dedicated LocationFilter).
Comment on lines
+991
to
997
| locations: | ||
| type: array | ||
| description: | | ||
| An array of **UN Location codes** to match with this subscription filter. If the array consists of more than one item - a logical **OR** is used between the values when matching. | ||
| An array of **UNLocationCode and/or facilitySMDGCode combinations** to match with this subscription filter. If the array consists of more than one item - a logical **OR** is used between the objects when matching. | ||
|
|
||
| If this property is empty, you will be notified for all `UNLocationCodes` that have changes you are authorized to see. | ||
| If this property is empty, you will be notified for all `locations` that have changes you are authorized to see. | ||
| items: |
There was a problem hiding this comment.
locations is defined twice (in Subscription and SubscriptionBodyWithSecret) but only the Subscription variant has minItems: 1. This inconsistency will confuse clients and tooling; align the constraints between the two schemas (and ensure they match the intended ‘empty means all’ semantics).
Comment on lines
+787
to
792
| locations: | ||
| type: array | ||
| minItems: 1 | ||
| description: | | ||
| An array of **UN Location codes** to match with this subscription filter. If the array consists of more than one item - a logical **OR** is used between the values when matching. | ||
| An array of **UNLocationCode and/or facilitySMDGCode combinations** to match with this subscription filter. If the array consists of more than one item - a logical **OR** is used between the objects when matching. | ||
|
|
There was a problem hiding this comment.
This change removes the previous UNLocationCodes / facilitySMDGCodes filter properties and replaces them with locations, which is a breaking change for the /subscriptions request and response schemas. If this spec follows SemVer (as indicated by the API-Version header examples), consider bumping the contract version and/or keeping the old fields as deprecated for backward compatibility.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+795
to
+823
| type: object | ||
| description: | | ||
| The UN Location code specifying where the place is located. The pattern used must be | ||
| It is possible to provide the `UNLocationCode`, the `facilitySMDGCode` or both when specifying the location. | ||
|
|
||
| - 2 characters for the country code using [ISO 3166-1 alpha-2](https://www.iso.org/obp/ui/#iso:pub:PUB500001:en) | ||
| - 3 characters to code a location within that country. Letters A-Z and numbers from 2-9 can be used | ||
| If multiple `UNLocationCode` or `facilitySMDGCode` values need to be provided, then they must be specified as separate objects in the `locations` array. | ||
|
|
||
| More info can be found here: [UN/LOCODE](https://unece.org/trade/cefact/UNLOCODE-Download) | ||
| example: NLAMS | ||
| facilitySMDGCodes: | ||
| type: array | ||
| description: | | ||
| An array of **Facility SMDG codes** to match with this subscription filter. If the array consists of more than one item - a logical **OR** is used between the values when matching. | ||
| **Condition:** At least one of `UNLocationCode` and/or `facilitySMDGCode` **MUST** be specified, both are also allowed. | ||
| properties: | ||
| UNLocationCode: | ||
| type: string | ||
| pattern: ^[A-Z]{2}[A-Z2-9]{3}$ | ||
| minLength: 5 | ||
| maxLength: 5 | ||
| description: | | ||
| The UN Location code specifying where the place is located. The pattern used must be | ||
|
|
||
| If this property is empty, you will be notified for all `facilitySMDGCodes` that have changes you are authorized to see. | ||
| items: | ||
| type: string | ||
| maxLength: 6 | ||
| description: | | ||
| The code used for identifying the specific facility. This code does not include the UN Location Code. | ||
| - 2 characters for the country code using [ISO 3166-1 alpha-2](https://www.iso.org/obp/ui/#iso:pub:PUB500001:en) | ||
| - 3 characters to code a location within that country. Letters A-Z and numbers from 2-9 can be used | ||
|
|
||
| More info can be found here: [UN/LOCODE](https://unece.org/trade/cefact/UNLOCODE-Download) | ||
| example: NLAMS | ||
| facilitySMDGCode: | ||
| type: string | ||
| maxLength: 6 | ||
| description: | | ||
| The code used for identifying the specific facility. This code does not include the UN Location Code. | ||
|
|
||
| The codeList used by SMDG is the [SMDG Terminal Code List](https://smdg.org/wp-content/uploads/Codelists/Terminals/SMDG-Terminal-Code-List-v20210401.xlsx) | ||
| example: ACT | ||
| The codeList used by SMDG is the [SMDG Terminal Code List](https://smdg.org/wp-content/uploads/Codelists/Terminals/SMDG-Terminal-Code-List-v20210401.xlsx) | ||
| example: ACT |
There was a problem hiding this comment.
The item schema for locations documents a MUST-condition (“At least one of UNLocationCode and/or facilitySMDGCode MUST be specified”), but the schema does not enforce it (an empty object {} currently validates). Add a schema constraint (e.g., anyOf with required: [UNLocationCode] / required: [facilitySMDGCode], or minProperties: 1) so tooling and validators reflect the documented requirement.
| The codeList used by SMDG is the [SMDG Terminal Code List](https://smdg.org/wp-content/uploads/Codelists/Terminals/SMDG-Terminal-Code-List-v20210401.xlsx) | ||
| example: ACT | ||
| The codeList used by SMDG is the [SMDG Terminal Code List](https://smdg.org/wp-content/uploads/Codelists/Terminals/SMDG-Terminal-Code-List-v20210401.xlsx) | ||
| example: ACT |
There was a problem hiding this comment.
Same as above: the locations.items schema states a MUST-condition for UNLocationCode/facilitySMDGCode, but the schema does not enforce it, so {} would validate. Add an explicit constraint (anyOf/oneOf with required, or minProperties) so validators and generated clients match the documentation.
Suggested change
| example: ACT | |
| example: ACT | |
| anyOf: | |
| - required: | |
| - UNLocationCode | |
| - required: | |
| - facilitySMDGCode |
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.
No description provided.