FI-1704: Prevent initialization from modifying arguments #99
Conversation
lib/fhir_models/fhir.rb
Outdated
| if doc.errors.empty? | ||
| FHIR::Xml.from_xml(contents) | ||
| else | ||
| FHIR::Json.from_json(contents) | ||
| end |
There was a problem hiding this comment.
I do not know if these are part of the 'public' API. If they were and someone used them they wouldn't have this info saved... which is unintuitive.
They aren't mentioned in the README, so maybe it's ok? Depends on how defensive we want this update to be...
There was a problem hiding this comment.
I don't consider them part of the public api, but it's easy enough to move the saving there.
lib/fhir_models/bootstrap/model.rb
Outdated
| class Model | ||
| extend FHIR::Deprecate | ||
|
|
||
| attr_reader :source_text, :source_hash |
There was a problem hiding this comment.
Do we really need to keep the source_hash? We are already increasing the memory footprint of a model by >2x by keeping the source text of the text it was hydrated from. If we store source_hash also, plus for each nested model in bundles, this update would increase the memory footprint of a FHIR resource instance by > 3x?
I could see us maybe needing to do this because of the primitive extension issue, but this is an expensive workaround if that is the case :(
There was a problem hiding this comment.
Yes, we need it because many resources are instantiated from hashes.
|
I'm concerned about resource usage implications of this on downstream users of this library. I think I'd be miffed if a library I used to parse json, for example, suddenly used 3x the memory for a feature that I don't use. note: I haven't actually instrumented this, but since we are essentially saving 2 more copies of the serialized version of the resource in the resource itself, it seems like thats what the approx magnitude usage would be... but maybe ruby objects are way more bloated and its not that big a hit |
3 participants
This branch prevents the hash argument from being modified in the FHIR models constructors. It also moves the Inferno monkey patch that stores the original source of a resource into this library.