dcarbone / php-fhir

Tools for consuming data from a FHIR server with PHP
Apache License 2.0
126 stars 40 forks source link

XML Attributes instead of Elements #126

Closed leonrenkema closed 3 weeks ago

leonrenkema commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

@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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@leonrenkema I have a PR to quickly enable you to configure the default serialization location for Elements: https://github.com/dcarbone/php-fhir/pull/142

Would something like this work for you?

leonrenkema commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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.