hapifhir / hapi-fhir

🔥 HAPI FHIR - Java API for HL7 FHIR Clients and Servers
http://hapifhir.io
Apache License 2.0
2.04k stars 1.33k forks source link

Version 3.6.0, according with the fhir resource structure definition, the priority of Task should be RequestPriority #1131

Open baopingle opened 5 years ago

baopingle commented 5 years ago

NOTE: Before filing a ticket, please see the following URL: https://github.com/jamesagnew/hapi-fhir/wiki/Getting-Help

Describe the bug In according with the structure definition of Task Resource in FHIR (3.0.1), the priority of it should be the RequestPriority not the inner enum TaskPriority.

Screenshots image

Environment (please complete the following information):

restevez-chs commented 5 years ago

i'm trying to diagnose this issue a little more (first time contributor, but have been using FHIR, and willing to help)...

I am looking at: hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/Task.java

@baopingle, your proposed change is this right?

/**
 * Indicates how quickly the Task should be addressed with respect to other requests.
 */
@Child(name = "priority", type = {CodeType.class}, order=9, min=0, max=1, modifier=false, summary=false)
@Description(shortDefinition="normal | urgent | asap | stat", formalDefinition="Indicates how quickly the Task should be addressed with respect to other requests." )
@ca.uhn.fhir.model.api.annotation.Binding(valueSet="http://hl7.org/fhir/ValueSet/request-priority")
protected Enumeration<RequestPriority> priority;

What problem are you seeing?

Seems that the TaskPriority is intended to be used: https://github.com/jamesagnew/hapi-fhir/blob/master/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/Task.java#L542

This would require an import of the codesystems package. But the problem is that I'm not seeing done in any of the other sibling classes to Task.java -- which makes me think the implementation is correct.

Another example which reinforces my thought that implementation is ok is: https://github.com/jamesagnew/hapi-fhir/blob/master/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/DeviceRequest.java#L669 It has a reference to RequestPriority and its an enum in DeviceRequest as well.

Consistent across these classes is the Binding.

@ca.uhn.fhir.model.api.annotation.Binding(valueSet="http://hl7.org/fhir/ValueSet/request-priority")

I think it's okay.

Thoughts??

metapink commented 5 years ago

From what I can tell there's no TaskPriority but only RequestPriority in the spec, therefore TaskPriority doesn't need to do anything fancy, or just be replaced with RequestPriority as you have show. This would mainly reduce code duplication

Thoughts??

baopingle commented 5 years ago

@restevez-chs , I fully agree with @ivanlebed comment. I don't understand why the codesystems package could not be imported in model package, is there any cycle dependency? I found there are a lot of duplicated enum/factory classes that have been defined in the codesystems. Seems there are something need to refactor.

restevez-chs commented 5 years ago

@baopingle this is great discussion on this issue...

we'll need @jamesagnew and other regular code contributors to chime in. i think our questions boil down to:

  1. Is the original implementation correct? https://github.com/jamesagnew/hapi-fhir/blob/master/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/Task.java#L1995 Should protected Enumeration<TaskPriority> priority; actually be protected Enumeration< RequestPriority> priority;
  2. Why is TaskPriority defined in Task.java? It appears to duplicate a RequestPriority: https://github.com/jamesagnew/hapi-fhir/blob/master/hapi-fhir-structures-dstu3/src/main/java/org/hl7/fhir/dstu3/model/codesystems/RequestPriority.java
grahamegrieve commented 5 years ago

I maintain the code generation. It's basically 2 separate unrelated code generaters here, the models and the code systems. The code systems are generated for convenience, but have never been integrated into the generated classes. It's been a long time but I can't recall ever intending to do that. It might be a good idea in principle to do it, but I think that users would not want it done in the already generated code and break everyone's existing code.

restevez-chs commented 5 years ago

@grahamegrieve hi. thanks for the reply. if i'm understanding your response correctly, then the answers to my questions above are:

  1. The original implementation is correct
  2. model and codesystems while seemingly related are really to be meant to be separate. and incorporating the org.hl7.fhir.dstu3.model.codesystems.RequestPriority into org.hl7.fhir.dstu3.model.Task would be a undesired breaking change.

so can this issue be closed with reason: "no issue. working as designed"?

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.