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

XML Attributes instead of Elements #126

Closed
leonrenkema opened this issue Jul 22, 2024 · 15 comments · Fixed by #143
Closed

XML Attributes instead of Elements #126

leonrenkema opened this issue Jul 22, 2024 · 15 comments · Fixed by #143
Assignees
Milestone

Comments

@leonrenkema
Copy link
Contributor

Hi, I tested the new implementation by generating the code based on R4. But I am seeing some strange generated XML.

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<Device id="123456">
    <meta profile="http://vzvz.nl/fhir/StructureDefinition/nl-vzvz-Device" />
    <identifier system="http://fhir.nl/fhir/NamingSystem/aorta-app-id" value="90000274" />
    <owner>
        <identifier system="http://fhir.nl/fhir/NamingSystem/ura" value="900000380" />
    </owner>
</Device>

For example the identifier element has the system and value as an attribute and not as a seperate element (see below for the generated code with the old generator). I don't think <identifier system="http://fhir.nl/fhir/NamingSystem/aorta-app-id" value="90000274" /> is valid FHIR Xml.

  <identifier>
    <system value="http://fhir.nl/fhir/NamingSystem/aorta-app-id"/>
    <value value="90000274"/>
  </identifier>
@dcarbone
Copy link
Owner

dcarbone commented Jul 22, 2024

Ah, it should be identical to the source. I now track whether elements with value primitives and primitives were seen as attributes or elements. You can also specify this location at set time: https://github.com/dcarbone/php-fhir-generated/blob/1623dc7d7c00a0cfd84f23b555e0d9b1cd0a368b/src/DCarbone/PHPFHIRGenerated/R4/FHIRElement/FHIRIdentifier.php#L361

If you can provide input xml that causes erroneous output, I can add it as a test case.

This is done as I have seen both structure formats, particularly if the element has extensions.

@leonrenkema
Copy link
Contributor Author

I created a small test that produces the following incorrect xml

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<Identifier system="http://example.com/identifier-system" value="example"/>

Small test with an assertion of what the output should be.

    public function testXmlAttributes()
    {
        $identifier = new FHIRIdentifier([
            'value' => 'example',
            'system' => 'http://example.com/identifier-system'
        ]);

        $xml = $identifier->xmlSerialize()->outputMemory();

        $this->assertEquals(
            '<Identifier>
  <system value="http://example.com/identifier-system"/>
  <value value="example"/>
</Identifier>',
            $xml
        );
    }

@dcarbone
Copy link
Owner

Ah, so that is effectively converting JSON to XML, and at that point I cannot predetermine which is the desired positioning.

I have thought about adding an xml position map parameter as a constructor arg for such occasions, or perhaps allowing these to be set globally in the config. Thoughts?

@leonrenkema
Copy link
Contributor Author

I am not sure if I understand your question, I am not converting JSON to XML. I use the library to create a model and serialize that to XML.

Or do you mean the part where I use the constructor? When I change it to this I get the same output.

$identifier = new FHIRIdentifier();
$identifier->setValue("example");
$identifier->setSystem("http://example.com/identifier-system");

It looks like the XML serialization does not follow the FHIR standard.

@dcarbone
Copy link
Owner

@leonrenkema Is this still an issue for you? I've released several updates with lots of various improvements. Additionally, the tests I've written specifically assert that the generated code is able to produce output identical to the input, so I'm not quite sure what else I can do here...

@leonrenkema
Copy link
Contributor Author

I tried again with the latest master checkout but I see the same behaviour.

What happens when you add the following test to your tests or something similiar?

$identifier = new FHIRIdentifier();
$identifier->setSystem("http://example.com/identifier-system");
$identifier->setValue("example");

$xml = $identifier->xmlSerialize()->outputMemory();

$this->assertEquals(
            '<Identifier>
  <system value="http://example.com/identifier-system"/>
  <value value="example"/>
</Identifier>',
            $xml
);

Now it generated <Identifier system="http://example.com/identifier-system" value="example"/> and that is not correct.

@dcarbone
Copy link
Owner

So, as mentioned if you want the xml serialization to appear in a particular location, you need to use the location enum: https://github.com/dcarbone/php-fhir-generated/blob/92535a6a7adb192288a21f5f4b4f6baab8e9b946/src/DCarbone/PHPFHIRGenerated/R4/FHIRElement/FHIRIdentifier.php#L355

This is because different sources have different serialization standards and I cannot enforce one without breaking it for another.

@leonrenkema
Copy link
Contributor Author

Ah, okay. But it is a whole load of work to add this to all the places where I call such methods. I always thought that the FHIR specification defines if it should be an element or an attribute but maybe it works different.

@dcarbone
Copy link
Owner

dcarbone commented Aug 26, 2024

It probably does, but I have seen both in the wild during the execution of my tests.

As you saw in v2, my solution was to just put the values in both places with the intent that receivers of this can just use whichever they were fixed to use.

I do agree that the current implementation is limited. I think a solution could be to define a new config field that lets you globally override the default. Thoughts?

@leonrenkema
Copy link
Contributor Author

leonrenkema commented Aug 28, 2024

The only exception I can find in an bundle we are using is the url attribute on an extension.
<extension url="http://hl7.org/fhir/StructureDefinition/humanname-assembly-order">

There are a few but not much. Can you not determine the correct default from the XSD you are parsing?

In the StructureDefinition it is coded like <representation value="xmlAttr"/> but I don't know if that information is also available in the XSD.

See https://www.hl7.org/fhir/elementdefinition-definitions.html#ElementDefinition.representation

@dcarbone
Copy link
Owner

dcarbone commented Aug 28, 2024

No, the XSD's do not specify placement in XML serialization.

I could add a way to configure the default when using the generator yourself.

I also was not aware if that ElementDefinition element. I can look into that, but I won't have time in the near future for larger updates.

@dcarbone
Copy link
Owner

@leonrenkema I have a PR to quickly enable you to configure the default serialization location for Elements: #142

Would something like this work for you?

@leonrenkema
Copy link
Contributor Author

I have tried it and I am afraid that it does not solve the problem.

With the default to attribute it places every primitivevalue on it's parent element I guess? Also somewhere the fri and 09:00:00 are lost.

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>\n
<Dosage sequence="1">
  <timing>
    <repeat dayOfWeek="mon" timeOfDay="08:00:00"/>
  </timing>
</Dosage>

With the default set to element the generated XML looks like this

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<Dosage>
    <sequence>
        <value value="1" />
    </sequence>
    <timing>
        <repeat>
            <dayOfWeek>
                <value value="mon" />
            </dayOfWeek>
            <dayOfWeek>
                <value value="fri" />
            </dayOfWeek>
            <timeOfDay>
                <value value="08:00:00" />
            </timeOfDay>
            <timeOfDay>
                <value value="09:00:00" />
            </timeOfDay>
        </repeat>
    </timing>
</Dosage>

With the version I now use it generates this and that is valid FHIR.

<Dosage>
  <sequence value="1"/>
  <timing>
    <repeat>
      <dayOfWeek value="mon"/>
      <dayOfWeek value="fri"/>
      <timeOfDay value="08:00:00"/>
      <timeOfDay value="09:00:00"/>
    </repeat>
  </timing>
</Dosage>

Maybe it is more complicated than a simple default.

@dcarbone
Copy link
Owner

Excellent, this is a great example of the issue. I believe this is two separate issues:

  1. When specifying attribute on a collection field, only the first element added will be allowed to be serialized as an attribute
  2. When serializing value containers, the value must always be an attribute.

@dcarbone
Copy link
Owner

@leonrenkema I have closed the other PR in favor of #143.

Using your provided example, I made this quick little test script:

<?php declare(strict_types=1);

require __DIR__ . '/../output/HL7/FHIR/R4/PHPFHIRAutoloader.php';

use HL7\FHIR\R4\FHIRElement\FHIRBackboneElement\FHIRDosage;
use HL7\FHIR\R4\FHIRElement\FHIRBackboneElement\FHIRTiming;
use HL7\FHIR\R4\FHIRElement\FHIRBackboneElement\FHIRTiming\FHIRTimingRepeat;
use HL7\FHIR\R4\PHPFHIRAutoloader;
use HL7\FHIR\R4\PHPFHIRXmlWriter;

PHPFHIRAutoloader::register();

$dosage = new FHIRDosage([
    FHIRDosage::FIELD_SEQUENCE => 1,
    FHIRDosage::FIELD_TIMING => [
        FHIRTiming::FIELD_REPEAT => [
            FHIRTimingRepeat::FIELD_DAY_OF_WEEK => [
                'mon',
                'fri',
            ],
            FHIRTimingRepeat::FIELD_TIME_OF_DAY => [
                '08:00:00',
                '09:00:00',
            ],
        ]
    ]
]);

$xw = new PHPFHIRXmlWriter();
$xw->openMemory();
$xw->setIndent(true);
$xw->setIndentString('  ');
$dosage->xmlSerialize($xw);
echo $xw->outputMemory(true);

Running this script produces the output:

<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
<Dosage>
  <sequence value="1"/>
  <timing>
    <repeat>
      <dayOfWeek value="mon"/>
      <dayOfWeek value="fri"/>
      <timeOfDay value="08:00:00"/>
      <timeOfDay value="09:00:00"/>
    </repeat>
  </timing>
</Dosage>

Hopefully this will resolve this issue for you! If so, please let me know and I will cut a release asap.

@dcarbone dcarbone linked a pull request Aug 30, 2024 that will close this issue
@dcarbone dcarbone added this to the v3.2.0 milestone Aug 30, 2024
@dcarbone dcarbone self-assigned this Aug 30, 2024
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

Successfully merging a pull request may close this issue.

2 participants