Fix info key

[teemukataja]

Fixes #168
This was attempted at #197 but was closed

This change introduces a solution to fix the issue with the info key and KeyValuePair, by conforming to JSON standards, in plain terms: info's value would be of type object.

An example of how this might look in practice is provided by @blankdots at #223 (comment)

This PR builds on top of #225 as it has the change made to .travis.yml.

[mbaudis]

Looks o.k., pending the parenthesis/comma fixes.

The comment about info is a bit spares; in the long run we should some specifications there (e.g. "do not put data which may facilitate re-identification of individuals here if used outside of controlled environment" and such).

[sdelatorrep]

The original requirement was a field for adding simple custom information, not complex structures. But if the community considers it should accept any strucutre, then we should go with this solution.
Another option is changing the structure to a dictionary as suggested by Juha in #168, but this would break the spec and should be presented to the Strategic Team.

[mbaudis]

@sdelatorrep See my comment on #168. Wouldn't break the schema if list of objects, or would it?

[sdelatorrep]

I think it wouldn't. An object in JSON can be whatever you want (e.g. a list of key-value pairs).

[juhtornr]

I think that this definitely should be fixed. The current solution is:

"info": [ 
  { 
    "key": "accessType", 
    "value": "PUBLIC"
  },{
     "key": "filterAlgorithm",
     "value": "CUSTOM"
  },{
    "key": "other",
    "value": null
  }
]

and the proposed solution is:

"info": 
  {
    "accessType": "PUBLIC",
    "filterAlgorithm": "CUSTOM",
    "other": null
  }

The proposed solution is much simpler and easier to parse.

[mbaudis]

@juhtornr I'm not against dictionary/hash instead of list (i.e. simply define as "object), if the values then can be of any type.

"info": 
  {
    "accessType": "PUBLIC",
    "filterAlgorithm": "CUSTOM",
    "data_use_conditions" : {
      "label" : "no restriction",
      "id" : "DUO:0000004"
    },
    "other": null
  }

[teemukataja]

To add to this, here is how simple it would be in the specification:
Instead of what it is currently:

info:
  description: 'Additional structured metadata, key-value pairs.'
  type: array
  items:
    $ref: '#/components/schemas/KeyValuePair'

KeyValuePair:
  type: object
  required:
    - key
    - value

Which results in the awkward form we want to get rid of for practical and aesthetic reasons.

It could be as simple as:

info:
  type: object
  description: Additional information
  example:
    accessType: PUBLIC
    something: else

^ See that we can give the example without enforcing the format

Looks like the following in the preview:

"info": {
  "accessType": "PUBLIC",
  "something": "else"
}

[mbaudis]

@teemukataja Sure; it just breaks the current info type ... So while this looks sensible & eases processing of the response, it is a strategic decision ...

That being said, we actually had this format already in our test implementation (somewhat erroneously, but didn't fix due to the direction this is moving ...).

{
  "info": {
    "version": "Beacon+ implementation based on the development branch of the ELIXIR Beacon project with custom extensions",
    "queryString": "datasetIds=dipg&referenceName=17&assemblyId=GRCh38&startMin=7,572,826&endMax=7,579,005&referenceBases=*&alternateBases=N&biosamples.biocharacteristics.type.id=icdot:C71.7&"
  },
  "beaconId": "progenetix-beacon",
  "exists": true,
...

[juhtornr]

@mbaudis I agree that from specification point of view this breaks the backwards compatibility. However we took a look how this is currently done in Beacon implementations and the conclusion is that:

• ELIXIR Beacon reference implementation v0.4 used the proposed object model, this was changed recently when updated to 1.0 specification but there's no live Beacon instances which would deliver the "right" response
• Beacon+ from Switzerland uses the proposed object model
• ELIXIR Luxemburg Beacon implementation uses the proposed object model
• GA4GH/DNAStack Beacon Network uses the proposed object model
• Beacon-python from ELIXIR-FI reverted the proposed object model in order to adapt to the specification

So the implementations are actually pretty much using the proposed model and therefore I would suggest that we simply "fix" the specification. Can you @mbaudis raise this to the strategic group?

[mbaudis]

@juhtornr Sounds convincing to me. I think we should just run this here, w/ approval from @jrambla, @garysaunders, @mcupak (and the Finnish team, which is a given...).

[mbaudis]

Approving, w/ requirement by others (see thread).

[jrambla]

Sorry gentlemen, is the proposed solution allowing for any payload in the "info"? or just key:value? Written from a mobile device, disregard brevity and typos, please. Escrit des d'un mòbil, disculpeu la brevetat i les errades. Escrito desde un móvil, disculpad la brevedad y las erratas.

…

________________________________ From: garysaunders <[email protected]> Sent: Friday, November 30, 2018 2:40:32 PM To: ga4gh-beacon/specification Cc: jrambla; Mention Subject: Re: [ga4gh-beacon/specification] Fix info key (#226) @garysaunders approved this pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#226 (review)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AHsiOnX36Kvm-NDm4sRVk11l4xA7ewdJks5u0TVQgaJpZM4YGUQu>.

[mbaudis]

My take:

• technically "any" (i.e key-value on the first level, where the value then can be of any type - not specifying this further, but documenting some sensible examples)
• content-wise note in the documentation that no content should be provided over this which would endanger the privacy protecting nature of the Beacon protocol, in Beacon instances with public access option
• for data delivery beyond technical metadata, we strongly recommend the use of the handover mechanism

So this is mostly usage definitions, not technical protocol ("here is a sharp knife, but ...").

[mcupak]

I like this change overall, and I think ideally, despite the troubles with parsing, I'd support going as far as extending this to cover arbitrary free form objects.

A few questions about this particular PR:

• This doesn't use additionalProperties: true - is that by design, or is it the default value? It's probably clearer to state it explicitly.
• I'd suggest using different examples since e.g. license does not make sense in that context.
• Can you rebase please?

I also second the concerns about backward compatibility, since this does break it. If we need to bump to 2.0, so be it, but I'd delay this to squeeze a few more things into the major release in that case.

[blankdots]

@mcupak related to this:

This doesn't use additionalProperties: true - is that by design, or is it the default value? It's probably clearer to state it explicitly.

in OpenApi specs https://swagger.io/specification/#schemaObject it is stated:

Consistent with JSON Schema, additionalProperties defaults to true.

[sdelatorrep]

Hi guys! I've got feedback from @jrambla and he approves this.

[teemukataja]

Re-based and updated as per suggestions

Done

[sdelatorrep]

Hi @teemukataja! I asked for some changes. Thanks!