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

proposing "warnings" for response meta #105

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mbaudis
Copy link
Member

@mbaudis mbaudis commented Jul 24, 2023

Warnings seem like a much needed item to understand responses where the server wants to indicate information about potential problems with the response which are not outright failures. Especially useful for debugging aggregators.

Warnings seem like a much needed item to understand responses where the server wants to indicate information about potential problems with the response which are not outright failures. Especially useful for debugging aggregators.
@mbaudis mbaudis requested review from redmitry and costero-e July 24, 2023 16:00
Copy link
Collaborator

@costero-e costero-e left a comment

Choose a reason for hiding this comment

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

Although I think this can be potentially helpful, in my opinion this is too much open and we would need to be more precise in the warnings we want to show in this object if we want this to be strictly developed by beacon implementers. I think that we need to find what are the things that we need to know about what is going wrong with the requests (request parameters? filters? any other?) so we can provide the guidelines. I approve this because of the importance of it but I would like this to be discussed.

@mbaudis
Copy link
Member Author

mbaudis commented Jul 31, 2023

@costero-e So I feel like changing string -> object (no further definitions, like info right now; or even using the info prototype). This would remove future hassle - one could design specific formats w/o breaking the spec. Agree? info or just type: object?

@costero-e
Copy link
Collaborator

I don't know if it's better to put it as an object or not, what I see is that if we don't specify all the cases in which we want the beacon to return a warning, this will be very generic and I don't know if it will be useful then.

@redmitry
Copy link
Collaborator

redmitry commented Aug 1, 2023

I don't know if it's better to put it as an object or not, what I see is that if we don't specify all the cases in which we want the beacon to return a warning, this will be very generic and I don't know if it will be useful then.

Unfortunately, the only place to report Beacon ERRORS is a dubious BeaconErorr object with an example "404" that has little sense to me. Beacon Network may use "warnings" as a placeholder for errors when we do have a response, but some beacons failed. Probably not best solution. I would extend beaconErrorResponse to use an array of BeaconErrors, but...

D.

@mbaudis
Copy link
Member Author

mbaudis commented Aug 7, 2023

@costero-e @redmitry I'm actually for keeping the format the way I've submitted (list of strings) since otherwise we run into read-out/interpretation problems, if ill-specified objects are used. So still IMO good as it is (as long as we don't spec warnings objects). The use of the BeaconError itself would be problematic since warnings usually wouldn't have a code... I'd rather would do something like message:..., url:... where the url could point to a doc page. Do we want to go tis way?

  BeaconWarning:
    description: >-
      Warning message by the beacon about possible problems in the query interpretation,
      special formatting of the response, access restrictions or similar.
    type: object
    required:
      - message
    properties:
      message:
        type: string
        examples:
          - This beacon does not support range-type variant queries.
          - This beacon only supports record-level granularity in its local network environment.
      url:
        type: string
        format: uri
        description: >-
          URL endpoint for further information, e.g. a documentation page or an authentication site.
        examples:
          - http://docs.genomebeacons.org/variant-queries/#beacon-range-queries

@redmitry
Copy link
Collaborator

redmitry commented Aug 7, 2023

@costero-e @redmitry I'm actually for keeping the format the way I've submitted (list of strings) since otherwise we run into read-out/interpretation problems,

Agree.

I would also add the property location:

"location" : {
    "type": "string",
    "format": "json-pointer"
}

As I mentioned before the error reporting is quite weak.
Json Schema recommends the format for schema errors reporting:
https://datatracker.ietf.org/doc/html/draft-bhutton-json-schema-01#section-12.4.2

D.

@costero-e
Copy link
Collaborator

@mbaudis The warning schema is more complete now, good job. On the other hand, I don't know if this is too technical. If this is oriented to users that don't have a lot of database or programming knowledge maybe these messages won't help them. But I guess every beacon developer can write the warning messages based on their needs so no problem.

@redmitry Can you provide an example of the location property, please?

@mbaudis
Copy link
Member Author

mbaudis commented Aug 16, 2023

@redmitry @costero-e location? Btw., have a look at the discussion there - maybe chime in? IMO it would be great to have the (geographic) location / geoLocation property in service-info. But a GeoLoaction object should anyway live somewhere & then be re-used across GA4GH standards; therefore I've had it for a while in {S}[B] ...

@redmitry
Copy link
Collaborator

@redmitry Can you provide an example of the location property, please?

The idea is to use "location" which should locate the problem in json document (basically the input, but BN may use it for the outputs also)
.
In the json schema terms it is a "instanceLocation" for example; "/query/filters/0"
Json Schema parsers should also provide the "absoluteKeywordLocation" e.g. "https://raw.githubusercontent.com/ga4gh-beacon/beacon-v2/main/framework/json/requests/beaconRequestBody.json#/properties/query/properties/filters"

But as you said probably too technical. The idea is to provide some additional insight about the error location.
For instance, Beacon Network may use it to point invalid metadata provided by beacons.

D.

@costero-e
Copy link
Collaborator

@redmitry Ok, I see, thank you. It is interesting if we have all the possible errors listed in the Json schema.
@mbaudis geolocation? What would we be using it for? To map where we have all the beacons or the info?

@mbaudis
Copy link
Member Author

mbaudis commented Aug 16, 2023

Yes. You can direcly use a GeoJSON object to project onto a map, geofence/query, get stats ... See the linked discussion. Was reminded again by the handmade ELIXIR map of beacons.
Now, while I wiuld prefer this as part of service-info (inherited by Beacon) we probably should demonstrate it in the network implementation (as additional parameter).

@jrambla
Copy link
Contributor

jrambla commented Jan 9, 2024

Sorry for jumping in late in the discussion.
I would like to see the actual use cases we are trying to solve here.
The provided examples are not compelling to me:

  • If a parameter could not be interpreted, an error should be returned or just a "false" response
  • The discrepancy in the granularity between request and response could be detected looking at the response header, and that is the purpose for having them there

In summary, if "warnings" is like a "hack" we should discuss which are the actual issues we are trying to solve and if "warnings" solve them or we need different solutions for different use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants