ga4gh / mme-apis

Documentation for the MatchmakerExchange APIs
https://github.com/ga4gh/mme-apis
34 stars 19 forks source link

Distinguish patient-initatied match requests in API #135

Open buske opened 7 years ago

buske commented 7 years ago

As discussed on today's call, one method for doing this is to add the contact type to patient.contact, with the burden on the receiving service to filter if they want to. @Relequestual raised the point that Decipher might not be okay with this, but needs to follow up.

While some sites currently allow users to specify preferences about what types of users they'd like to match against (e.g., researchers not matching against patients), this would not be enforced over the MME.

buske commented 7 years ago

I have two proposals:

  1. Add a top-level scope parameter to the request, outside of the patient object, to represent the scope with which the returned information will be displayed: clinical, research, or public. For example, all PhenomeCentral requests would have "scope": "research". For MyGene2, requests in which responses will be visible to patients would be public scope, but requests coming from researchers would be research scope. This emphasizes how broadly the data will be shared. The request API would then look like:

    {
      "scope": "clinical"|"research"|"public",
      "patient": {...}
    }
  2. Add a patient.contact.roles list, which is 0 or more of clinician, researcher, or participant, and describes the contact person for the patient record. For PhenomeCentral, we don't know whether users are clinicians or researchers, so we would probably just use researcher for everyone. MyGene2 would use researcher or participant according to the person who is initiating the request. This emphasizes who the contact person is. The request API would then look like:

    {
      "patient": {
        "contact": {
          "name": "Dr. Smith",
          "href": "mailto:smith@hospital.org",
          "scope": ["clinician"|"researcher"|"participant", ...]
        }
      }
    }

In both of these proposals, we can probably ignore the clinical/research distinction for now as well.

jxchong commented 7 years ago

I think option 2 makes more sense as it isn't open to the misinterpretation that the querying service will/will not be protecting the data in specific ways.

For future-proofing purposes, I would make "scope" in the patient.contact.roles list include "clinician" as a distinct valid value separate from researcher even if right now they're effectively the same. One could imagine sending slightly different messages/presenting the information in different ways to your own user depending on the identity of the contact person.

Another thought from the MME meeting at ASHG:

Some ideas had been floated such that families could participate in a moderated fashion, i.e. the contact information that goes out is of their designated clinician and the clinician decides whether to send the match information on to the family. Since we're discussing this, would that form of participation be scope=clinician or scope=participant (this is all hypothetical of course)? Arguably this isn't relevant since it could easily be happening now: any clinician could already be submitting cases to MME and then telling their patient about matches.

Relequestual commented 7 years ago

I would agree option 2 seems more sane. Similar to PhenomeCentral, we don't know if the user is a clinician or researcher, but the majority of time they will be primerilly a clinician.

I'll float this issue at our weekly DECIPHER meeting when I'm next able to do so. I'm hoping clearer communication will resolve the issue, as we haven't had enough time to discuss it fully.

Relequestual commented 7 years ago

Misscommunication!

DECIPHER wouldn't collect this information, but if any system wanted to send it, we would could filter it out before displaying. The confusion was over what "recieving" meant. I meant from other systems, but it was understood as collection on our side. So that's fine =] (I guess this is more the D.O.B discussion)

jxchong commented 7 years ago

@Relequestual That would be great. Do we/you send a revised statement of understanding? It would be awesome if, once the API supports this, we can just send all requests and then other nodes can choose to filter out the ones they don't wish to process.

Relequestual commented 7 years ago

@jxchong We still wouldn't want to get requests from or match on data entered by patients. How that information comes out in the API is not too important, imho.

It was more if personably identfiyable information (like DOB or name) was included in the request or response, that's not a problem, as we can simply filter that information out. This is an acceptable solution for us.

jxchong commented 7 years ago

Wait, I'm confused. So regardless of whether we add this field to the API, you still want us to pre-filter requests so that you only get sent requests from C/R? Or are you ok with being sent all requests (with this new field), and then filtering yourself? We're not going to be sending name or DOB anyway.

Relequestual commented 7 years ago

Sorry, I haven't been clear. We don't WANT to recieve requests originating from patients but we could if we resolve some issues... we would have to define IN the API what response is OK for the instance where we say "sorry we can't help you" (HTTP status code I expect).

We would have to confirm it works correctly in DECIPHER before the new version of the API was implemented, but that's more our problem. The "new" version of the API still has a fair amount of work to do, so realistically I don't see this sort of change being rusehed through.

--

As an aside, we probably need some reclarification on API versioning and scope. I think, and I don't know if @buske would agree, but there are a few issues which could be addressed by a 2.0 release, but I know that a lot of disucssion agreed on the project wanting a reference server with the 2.0 release, and that's a LOT of work, especially if new fields are being added. Personally I wouldn't mind if 2.0 came out before the reference server was completed.

buske commented 7 years ago

Thanks for your feedback. One last question before I make a PR. What is the best approach if PhenomeCentral or Decipher knows that the user is a clinician or researcher, but isn't sure which? Do we just choose one? What about early GeneMatcher users, from before they selected what category of user they were? Technically they could be non-specialists, right?

@Relequestual To summarize, we'll proceed under the assumption that servers are free to send DECIPHER all match requests, as long as they were flagged accordingly so that DECIPHER can filter them out. I'd suggest 403 Forbidden, 204 No Content, or just a normal 200 OK with an empty list of matches.

buske commented 7 years ago

@jxchong To respond to your earlier question, I see it as being scope/role=clinician if the contact person is a clinician, even if the information is patient-entered. We could imagine communicating separately the source of each phenotype (clinician, patient, social worker, several of the above).

Relequestual commented 7 years ago

I agree with @buske's summary, assuming that the requestor also includes the version of the API they are sending. I say this because currently we expect and explicitly only support version 1.0, so saying the request is 1.0 but from a patient, may not be picked up. I THINK the request would be rejected, given how strict we are with erroneous attributes, but I haven't checked.

buske commented 7 years ago

That's a very good point, @Relequestual. If we want the API to be backwards-compatible, we actually need to loosen up on our requirement that unspecified fields be prefixed with an underscore. This can be a recommendation, but we cannot invalidate a request or response for having additional fields that don't start with an underscore (I'm thinking of the reference-server jsonschema). Otherwise, adding a new optional field in v1.1 makes a request invalid to a v1.0 server (and a v1.1 response invalid to a v1.0 request), defeating the whole point.

fschiettecatte commented 7 years ago

Sorry for the lateness, been out for a week. Option (2), linked to the patient record rather than the request. Option (1) does not work for responses anyway from what I can tell.

For GeneMatcher users who created accounts before self-identification, we marked them as 'clinician'|'researcher', and sent out an email to all asking them to update that information.

Relequestual commented 7 years ago

@buske That's not actually the point I was trying to make, but actually a way more important one! Good spot on that.

I agree with your statement that we shouldn't invalidate requests which have additional fields that are not prefixed with an underscore, but we need to state that in the API in the section that mentions additional fields, which would then also bump to 2.0. I'm happy for us to bump to 2.0 if the only breaking change is that discussed above. I think DECIPHER is the only service out of the current active services which is that strict, but we should at the least confirm this with the other systems.

We COULD just make the change (that additional fields are allowed), and still make it 1.1, but that would be against Semantic Versioning. Personally I feel we have to go all in with semver, and just bump to 2.0 with only that one breaking change.

fschiettecatte commented 7 years ago

@Relequestual can you explain to me why this would break semver? Is the goal of semver to have compatibility across all minor versions? So adding "scope" would break things because I could not send a 1.1 request to a 1.0 server? I feel there is something I am missing here...

Relequestual commented 7 years ago

Correct. According to 1.0, additional fields must be prefixed with underscore. Adding a any new field to the spec must therefore trigger major version bump as we didn't make adding new fields not prefixed with underscore a posibility for 1.0. So adding "scope" would break things because I could not send a 1.1 request to a 1.0 server? Exactly so.

Relequestual commented 7 years ago

I've taken a much more legalistic approach to the implementation of the API than others. If it doesn't say you can do something, then you can't, strictly speaking.

buske commented 7 years ago

So, while we added this helpful suggestion that non-standard fields be prefixed with an underscore, it gets in the way of cross-compatibility if you actually try to enforce this as a requirement.

Therefore, we should just get rid of this suggestion and anyone actually enforcing it should stop. @Relequestual, underscores being required is actually not part of v1.0a as we tagged it. It is part of the documentation improvements we added to clarify the API for people using the v1.0 version, exactly the reason that having a release/v1.0 branch is important. I suggest we just remove that section from the release/v1.0 docs and DECIPHER loosens up that constraint. Is everyone okay with this?

Relequestual commented 7 years ago

@Relequestual, underscores being required is actually not part of v1.0a as we tagged it.

In which case, I agree, and we won't be breaking semver by making that change.

Indeed, it does underline the usefulness of sticking hard and fast to gitflow!

Relequestual commented 7 years ago

I think it could also be a huge benefit to include the JSON Schema document you've created for the reference server, even if it's just as a guide and not to be taken as hard and fast rules initially.