Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Epcis query parameters in query string #434

Merged
merged 5 commits into from
Jun 7, 2022

Conversation

mgh128
Copy link
Collaborator

@mgh128 mgh128 commented Jun 5, 2022

I've proposed an updated version of openapi.yaml (and regenerated openapi.json) in which I have included the fixed-name EPCIS query parameters (e.g. eventType, EQ_bizStep, GE_eventTime ) within #/components/parameters/
and have referenced these from the GET methods of most REST endpoints that end in /events

I did find that OpenAPI 3.1.0 already supports "style":"pipeDelimited" with "explode": false, which appears to be exactly the combination we need when expressing something like EQ_bizStep=shipping|receiving or EQ_disposition=active|in_transit in the URI query string. We had not been using this until now - but from the testing I did using the Swagger editor, this seems to be the correct approach and did not require us to describe some clumsy workaround.

Unfortunately, I can't find any support for variable parameter names within OpenAPI 3.1.0, which is clearly problematic since around half of the EPCIS query parameters shown in the table in section 8.2.7.1 do not have fixed names but instead have flexible names in which _uom, _type or _fieldname etc. are not literal strings but might be substituted with something like _CEL, _possessing_party, etc., respectively.
I did see a mention in https://spec.openapis.org/oas/v3.1.0#patterned-fields-2 of 'patterned fields which declare a regex pattern for the field name.' - but no practical examples of how to formulate these correctly, nor in the Swagger editor.

I noted also that the current query-schema.json appears to be treating such italicised _uom, _type, _fieldname as though they were a literal part of a fixed parameter name (e.g. literally EQ_source_type whereas the actual parameters are EQ_source_possessing_party, EQ_source_owning_party or EQ_source_location), so I'm not convinced that query-schema.json as it's currently formulated will be able to validate such flexibly-named query parameter fields - but nor do I have a more elegant solution about how to do that in JSON Schema.
I think there might be an opportunity to include descriptions and enumerations within query-schema.json to make that friendlier to any tools that might display such information.

While preparing this, I did notice a number of other glitches, both in openapi.yaml and in the table of section 8.2.7.1 of the EPCIS specification, so I've informed @CraigRe about those.

I hope you feel that this is helpful and aligns with your intention to make more of the EPCIS Query language discoverable via the OpenAPI interface, using tools such as the Swagger Editor. I used the Swagger Editor while developing these enhancements to openapi.yaml.

If you're aware of a way to group the set of references to the various fixed-name EPCIS Query parameters, please make that improvement, rather than having to list them all each time. However, when listing them all, I did take care to omit the one that could conflict with the value of a field expressed within the endpoint URI path information, e.g. within the GET method for endpoint /bizSteps/{bizStep}/events I have omitted the reference to #/components/parameters/EQ_bizStep.

Looking at EPCIS §12.7.3, I'm wondering whether we should have more explicit text about how an implementation should handle the situation where the same parameter (e.g. bizStep) could be expressed in both the URI path information and the URI query string. Should that throw a QueryParameterException or does the value expressed in the path information override the value(s) expressed in the query string? - or vice versa? - or should the implementation merge all such specified values into a list/array of alternative values, sourced from the URI path information and the URI query string?

@jmcanterafonseca-iota
Copy link
Collaborator

@mgh128 I am concerned about the maintainability of the spec with this approach.

For instance, why the bizSteps are duplicated instead of getting them from the JSON Schema and injected as we did in other cases? Same with dispositions ...

@mgh128
Copy link
Collaborator Author

mgh128 commented Jun 6, 2022

Hi @jmcanterafonseca-iota
If you agree in principle with the output (openapi.json), we can then adjust your schema injector code to insert those enumerations from the JSON Schema, to improve maintainability, rather than hard-code them in openapi.yaml

@jmcanterafonseca-iota
Copy link
Collaborator

concerning the issue on duplicated parameters I think /bizSteps/shipping/events?EQ_bizStep=delivering should raise an error i.e. you cannot query events by bizStep when you are getting only those that match a particular bizStep through an explicit endpoint. That should be clarified in the spec and in the OpenAPI so that such parameter is not allowed in the corresponding endpoint.

minItems: 1
items:
type: string
format: URI
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
format: URI
format: uri

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jmcanterafonseca-iota - I've now corrected those upper-case URI to lower-case uri

in: query
required: false
schema:
type: integer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could quantity be a float?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! @jmcanterafonseca-iota . Alerting @CraigRe to another inconsistency in the draft spec.
The UML class diagrams show quantity as xsd:double
The ontology files show quantity as xsd:decimal
Section 7.3.3.1 (QuantityElement) shows quantity as 'Decimal' - though it's unclear whether that is actually xsd:decimal
Section 8.2.7.1 (SimpleEventQuery) shows EQ_quantity expecting an 'Int' , while EQ_quantity_uom is a List of String

So much for any consistency within the draft spec!

minItems: 1
items:
type: string
format: URI
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
format: URI
format: uri

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I've corrected that now

minItems: 1
items:
type: string
format: URI
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check all

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now changed all format: URI --> format: uri

items:
type: string
anyOf: [
enum: [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cross reference JSON Schema and inject

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - we should populate the enum using the injector tool

Copy link
Collaborator

@domguinard domguinard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes render well for me and make things clearer indeed. For me this is not a must do right now but a very nice improvement if we can get it in without further delaying the ratification

Copy link
Collaborator

@jmcanterafonseca-iota jmcanterafonseca-iota left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgh128 please could you check?

minItems: 1
items:
type: string
anyOf: [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to be JSON

items:
type: string
anyOf: [
enum: [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to be JSON

type: array
minItems: 1
items:
type: string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uri

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I'll check these. On the positive side, my misuse of JSON within YAML has nevertheless resulted in the intended result in openapi.json - though I agree that I should correct the openapi.yaml - and will also check the type at line 5235 - though right now, I'm preparing a pull request for some gaps and glitches I noticed in query-schema.json

schema:
type: array
minItems: 1
items: {type: string,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is JSON

@jmcanterafonseca-iota
Copy link
Collaborator

jmcanterafonseca-iota commented Jun 7, 2022

a last overall comment is,

I am still concerned about the maintainability of the approach of enumerating query parameters

probably future versions of the standard should consider just having a single query parameter, for instance q , and q can be specified to contain an expression according to certain grammar, for example

GET /events?q=bizStep=shipping;eventTime>=22-05-2022T12:00

@jmcanterafonseca-iota
Copy link
Collaborator

I've just checked and it is pretty easy to inject bizSteps in the Schema

for instance

BizStepCollection:
      type: object
      required:
        - '@context'
        - 'type'
        - member
      properties:
        '@context':
          $ref: '#/components/schemas/LDContext'
        'type':
          type: string
          enum: 
            - 'Collection'
        member:
          type: array
          items:
            $ref: 'https://gs1.github.io/EPCIS/EPCIS-JSON-Schema.json#/definitions/bizStep'
          uniqueItems: true

@mgh128
Copy link
Collaborator Author

mgh128 commented Jun 7, 2022

a last overall comment is,

I am still concerned about the maintainability of the approach of enumerating query parameters

probably future versions of the standard should consider just having a single query parameter, for instance q , and q can be specified to contain an expression according to certain grammar, for example

GET /events?q=bizStep=shipping;eventTime>=22-05-2022T12:00

I'm not sure that q=bizStep=shipping will even be valid in a URI query string. Would everything that processes a query string actually extract bizStep=shipping;eventTime>=22-05-2022T12:00 as the value of q ?
It certainly diverges from the more usual approach of ?key1=value1&key2=value2... where key1, key2 etc. would be the query parameters and value1, value2 might be pipe-delimited, when expressing a list of alternative values.

Even if a future version were to use a single query parameter, I expect that the second = in your example would need to be percent-encoded, together with many of the other symbol characters such as >= when these are considered to be the value of q.

@jmcanterafonseca-iota
Copy link
Collaborator

jmcanterafonseca-iota commented Jun 7, 2022 via email

@mgh128
Copy link
Collaborator Author

mgh128 commented Jun 7, 2022

thank you Mark. once you fix that I can make a final follow-up PR that fixes the issue of reusability of bizStep, disposition, types, etc.

Thanks in advance, @jmcanterafonseca-iota - I think I have now reformulated my JSON within YAML as YAML.

@CraigRe CraigRe merged commit 3a2a80f into master Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants