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

generate FHIR::Resource and FHIR::DomainResource, include by inheritance #6

Open
asross opened this issue Jun 9, 2016 · 5 comments

Comments

@asross
Copy link
Contributor

asross commented Jun 9, 2016

For some of the things it might be nice to do with FHIR::Reference resources, it might be helpful to reproduce some of the inheritance hierarchy specified in the XML schemas / FHIR docs, kind of like this:

module FHIR
  # core FHIR::Model supersuperclass
  class Model
    # anything we want available on every resource
  end

  class Element < FHIR::Model
    attr_accessor :id                   # 0-1 id
    attr_accessor :extension            # 0-* [ Extension ]
  end

  class BackboneElement < FHIR::Element
    attr_accessor :modifierExtension    # 0-* [ Extension ]
  end

  # generated Resource and DomainResource superclasses
  class Resource < FHIR::Model
    attr_accessor :id                   # 0-1 id
    attr_accessor :meta                 # 0-1 Meta
    attr_accessor :implicitRules        # 0-1 uri
    attr_accessor :language             # 0-1 code
  end

  class DomainResource < FHIR::Model
    attr_accessor :text                 # 0-1 Narrative
    attr_accessor :contained            # 0-* [ Resource ]
    attr_accessor :extension            # 0-* [ Extension ]
    attr_accessor :modifierExtension    # 0-* [ Extension ]
  end

   # generated resource classes
   class Bundle < FHIR::Resource
    attr_accessor :type          # 1-1 code
    attr_accessor :total         # 0-1 unsignedInt
    attr_accessor :link          # 0-* [ Bundle::Link ]
    attr_accessor :entry         # 0-* [ Bundle::Entry ]
    # ...

     class Link < FHIR::BackboneElement
        # ...
     end
   end

   class Patient < FHIR::DomainResource
    attr_accessor :identifier           # 0-* [ Identifier ]
    attr_accessor :active               # 0-1 boolean
    attr_accessor :name                 # 0-* [ HumanName ]
    attr_accessor :telecom              # 0-* [ ContactPoint ]
    # ...

    class Contact < FHIR::BackboneElement
      # ...
    end
   end
end

I'm not sure how challenging this would be to implement -- my intuition is that it could actually make some of the generation code simpler, because there would be more of a 1-1 mapping between the XML schemas and the generated model files -- but it might be useful.

@arscan
Copy link
Member

arscan commented Jun 9, 2016

I spoke with @jawalonoski and @andrequina about doing this yesterday.

The reason why this was implemented as a flat structure is that the order of the elements within the models is important for serialization/deserialization, and using inheritance caused problems with controlling that order. Is there a functional advantage to reorganizing the models to match the FHIR hierarchy that couldn't be solved with a simple workaround? Could the reference resolving issue that you have be solved by seeing if the model responds to contained, instead of checking the type?

I agree that it would be logically cleaner to use the same hierarchy, but there were some issues that came up when we did that, so I think we'd need some concrete benefits to justify tracking down those issues.

What do you think? Let me know if I mischaracterized the problem @jawalonoski.

@asross
Copy link
Contributor Author

asross commented Jun 10, 2016

The reason why this was implemented as a flat structure is that the order of the elements within the models is important for serialization/deserialization, and using inheritance caused problems with controlling that order.

Hmm, that makes sense, but could you explain what the element ordering problem is? At first glance it seems like it shouldn't matter. Although I'm realizing there might be an issue with constants (although I think it's resolvable).

Is there a functional advantage to reorganizing the models to match the FHIR hierarchy that couldn't be solved with a simple workaround? Could the reference resolving issue that you have be solved by seeing if the model responds to contained, instead of checking the type?

We could definitely do a simple workaround. I think when I was testing I found that FHIR::Bundle responded to contained, but that might have changed with this latest version, so that could work.

However, there might still be a functional advantage, e.g. if you wanted to define some methods just on FHIR::DomainResources. If there is an unsolvable problem with attribute declaration we could keep the inheritance structure but without the attr_accessors (if we can enumerate enough compelling use-cases to really justify saying there's a functional advantage, which I haven't).

@asross
Copy link
Contributor Author

asross commented Jun 10, 2016

I'm not sure if this would help solve the problem, but if the issue stems from the fact that the order is actually specified twice (once in METADATA, once in the attr_accessor statements), you could do something like the following:

class FHIR::Model
  def self.attrs
     @attrs ||= {}
  end

  def self.attr(name, metadata)
     self.attr_accessor(name)
     self.attrs[name] = metadata
  end

  def self.inherited(subclass)
     subclass.instance_variable_set(:@attrs, @attrs.deep_dup)
  end
end

class FHIR::Resource < FHIR::Model
   attr 'id', 'type'=>'id', 'min'=>0, 'max'=>1
   attr 'meta', 'type'=>'Meta', 'min'=>0, 'max'=>1
   attr 'implicitRules', 'type'=>'uri', 'min'=>0, 'max'=>1
   # ...
end

It would make the models a bit more concise, although it might require removing the path element (which does seem to be derivable from the class name and the attribute name, although I could be missing something).

@asross
Copy link
Contributor Author

asross commented Jun 10, 2016

Sorry to throw out so many half-thought-through proposals, but could also consider module inheritance, maybe using ActiveSupport::Concern, to resolve any thorniness around inheritance of attributes:

module FHIR::Model
  extend ActiveSupport::Concern
  included do
    attr 'id', 'type'=>'id', 'min'=>0, 'max'=>1
    # ...
  end
end

module FHIR::Resource
  extend ActiveSupport::Concern
  include FHIR::Model
end

module FHIR::DomainResource
  extend ActiveSupport::Concern
  include FHIR::Resource
end

module FHIR::BackboneElement
  extend ActiveSupport::Concern
  include FHIR::Model
end

class FHIR::Patient
  include FHIR::DomainResource
end

@arscan
Copy link
Member

arscan commented Jun 14, 2016

Unfortunately we don't have a ton of resources to put against giving this a shot right now. If you'd like to try some of these ideas out and open a PR, we'd happily merge it in if it doesn't cause any problems.

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

No branches or pull requests

2 participants