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

Correctly serialize STU3 ConceptMap and ActivityDefinition element order #683

Merged
merged 1 commit into from Jul 14, 2017

Conversation

Projects
None yet
3 participants
@CarthageKing

CarthageKing commented Jul 3, 2017

This PR fixes #634.

The cause was the paths in the ChildOrder annotation on these resources used '[x]'. However, the forcedOrder logic in BaseRuntimeElementCompositeDefinition.scanCompositeElementForChildren() method didn't account for the '[x]'. This leads to e.g. in ConceptMap, the sourceUri/sourceReference element being written first before url element, leading to a validation failure later on if the serialized resource is validated.

I did a scan of the code and it seems like ChildOrder annotation is only used in the STU3 model classes. Moreover, it seems only ConceptMap and ActivityDefinition resources are the ones who have paths that have '[x]'.

Since the fix was made to generated code, I think a more permanent fix to this would be to update the FHIR code generator not to use '[x]' in ChildOrder annotation paths.

michael.i.calderero
remove '[x]' from paths in @ChildOrder annotation because they mess up
'forcedOrder' resolution in
BaseRuntimeElementCompositeDefinition.scanCompositeElementForChildren()
method
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 3, 2017

Coverage Status

Coverage decreased (-0.003%) to 76.905% when pulling d26fba1 on CarthageKing:bugfix-issue634 into 294d080 on jamesagnew:master.

coveralls commented Jul 3, 2017

Coverage Status

Coverage decreased (-0.003%) to 76.905% when pulling d26fba1 on CarthageKing:bugfix-issue634 into 294d080 on jamesagnew:master.

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Jul 13, 2017

Owner

Ah this is interesting. Will require a modification to the source generator too, which shouldn't be too hard. I'm going to leave this open until that fix is made, just to track that too.

Owner

jamesagnew commented Jul 13, 2017

Ah this is interesting. Will require a modification to the source generator too, which shouldn't be too hard. I'm going to leave this open until that fix is made, just to track that too.

@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Jul 13, 2017

Owner

(Incidentally I can take care of that part.. it's easy enough, especially since you've supplied a unit test)

Owner

jamesagnew commented Jul 13, 2017

(Incidentally I can take care of that part.. it's easy enough, especially since you've supplied a unit test)

jamesagnew added a commit that referenced this pull request Jul 14, 2017

@jamesagnew jamesagnew merged commit ef3f720 into jamesagnew:master Jul 14, 2017

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.003%) to 76.905%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jamesagnew added a commit that referenced this pull request Jul 14, 2017

@CarthageKing CarthageKing deleted the CarthageKing:bugfix-issue634 branch Jul 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment