Add support for root attributes propagation

[portellaa]

Sometimes we want some attributes in the root node of our xml that don't should or need to be in the model, for example the attributes for the namespace or schema.

This pull request uses the existing attributes in the boxes to add those attributes to the root node.

Also, we can now infer the root node name instead of providing it in the encode method, making it optional in the API.

I'm not sure if this is the best place, or add them as a configuration for the coders instead of use them all the time we call the encode method.

It would be nice if those attributes were moved to the coder configuration, so we could have the clean coding methods and looking more like the Apple JSON coders. What you think?

[MaxDesiatov]

Hi @portellaa, thanks for the PR! Would you be able to give an example of XML and their corresponding codable types that show how exactly this would be a useful addition? Also, these changes are not tested and I'm wary of decreasing the test coverage of the library, could those examples then be adapted as new test cases submitted in this same PR?

[portellaa]

hi @MaxDesiatov of course,

Imagine the following model:

struct ServiceRequest: Codable {
    let type: String
    let id: String
    let element1: String
    let element2: String
}

Now imagine that the API requires that the xml generated, contains the namespace (xmlns), prefix (xmlns:xsi) and schema (xmlns:xsd), in the root element, you would have to generate something like this:

<?xml version="1.0"?>
<ServiceRequest xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:xsd="http://www.w3.org/2001/XMLSchema" Type="Test" ID="id1" xmlns="http://www.nrf- arts.org/IXRetail/namespace">
<Element1>Test</Element1>
<Element2>Test</Element2>
</CardServiceRequest>

I don't want (and/or need), that my model contains the xmlns, xmlns:xsd and xmlns:xsi properties, this is a coding requirement, it will not change with the model.

Does it sounds right to you?

Cheers 🍻

[MaxDesiatov]

Ok, that makes sense in general, thanks for the clarification. This only touches encoding though, how would you like those attributes to be handled on the decoding side?

[portellaa]

I can handle them the same way, provide a rootAttributes in the decoding. or perhaps in the userInfo. I wasn't sure of the best approach, so i didn't implemented that part 🤔

Do you have an idea? I just don't want the api to get weird of out of the "standard" with that attributes thing? Do you think it makes sense to decode and send them as a decode result? I mean, i think they makes sense to be used by the decoder, but not sure if propagate them as a result makes sense.

[portellaa]

Should i create a test case for those tests? Or should i include them in the one of the existing ones? 🤔 This should be a separate one, right?

[MaxDesiatov]

I can handle them the same way, provide a rootAttributes in the decoding. or perhaps in the userInfo. I wasn't sure of the best approach, so i didn't implemented that part 🤔

That makes sense. I'm just wondering, if in the future this is to be implemented on the decoding side, we'd probably want the API to be consistent for both encoding and decoding and hopefully also intuitive enough. If we can anticipate how it would look for decoding, making it consistent would be much easier.

Adding a test case probably with the example you gave would be great, thank you!

[codecov[bot]]

Codecov Report

Merging #160 into master will increase coverage by 0.21%. The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
+ Coverage   72.84%   73.06%   +0.21%     
==========================================
  Files          43       43              
  Lines        2287     2309      +22     
==========================================
+ Hits         1666     1687      +21     
- Misses        621      622       +1     

Impacted Files	Coverage Δ
Sources/XMLCoder/Encoder/XMLEncoder.swift	83.46% <72.72%> (-0.94%)	:arrow_down:
Sources/XMLCoder/Auxiliaries/XMLCoderElement.swift	95.56% <77.77%> (+0.04%)	:arrow_up:
Sources/XMLCoder/Auxiliaries/KeyedStorage.swift	95.55% <0.00%> (+0.20%)	:arrow_up:
...s/XMLCoder/Encoder/XMLKeyedEncodingContainer.swift	74.01% <0.00%> (+2.36%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d4d50d2...1b7cb2a. Read the comment docs.

[portellaa]

Sorry i was working on a different thing and didn't had time to finish this. I'm not a fan on how it is right now, because i had to repeat the code that encodes the keys based on the strategy for the root key, but ideally this should be done in Keyed container, but i didn't had time to implement it and i hasn't sure you want that.

[MaxDesiatov]

To pose this from a different perspective, are there any other root attributes other than xmlns stuff that should be handled by this API instead of specifying those attributes on a root model type directly? If the answer is yes, then the current API is probably the best available option.

If only possible root attributes a user would like to handle are the xmlns namespace attributes, and the rest can be handled perfectly well on the root model type, maybe the API should be more focused on making it easier to work with XML namespaces instead of just plainly serializing a given array of attributes. I hope this makes sense.

(I don't have an answer to these questions, I'm just looking for more feedback before potentially merging it as is)

[portellaa]

i agree that this is not the best way to handle this. as far as i know from xml (i haven't used it for a 10y or more, i had to use it for a freelance proof of concept), but when you specify xmlns:<something> this is can then be used in the all the xml elements, like this example extracted from W3 schools:

<root xmlns:h="http://www.w3.org/TR/html4/"
xmlns:f="https://www.w3schools.com/furniture">

<h:table>
  <h:tr>
    <h:td>Apples</h:td>
    <h:td>Bananas</h:td>
  </h:tr>
</h:table>

<f:table>
  <f:name>African Coffee Table</f:name>
  <f:width>80</f:width>
  <f:length>120</f:length>
</f:table>

</root>

as far i understood from https://www.w3.org/TR/xmlschema-1/ those xsd (or xs) and xsi are standard prefixes commonly used, where xsd stands for xml schema definition and xsi for xml schema instance.

i remember from the times i use to use SOAP and xml envelopes (12y ago), those are attributes were very important, because they were used for validation of the document, i believe they may not be that important right now since nowadays most of the people and services users JSON, protobuf and so on, so...

i've be reading more on this subject and more than before, now i think this is a bad addition because it will induce people in error if they expect schema parsing and validation in the coders. i believe this can be useful for other things, you can always add something extra that your service requires at the root level, but at least for this case of namespace and schemas, it isn't the best solution. i do believe that the rootKey addition to infer the root element is a nice one, although it's not the best solution i'm afraid, at least is not like Swift JSON and plist coders does it. if you think that is interesting, i can move it to another PR just with that.

that said, feel free to not accept this. at least it can be used for discussion, but probably move this to an issue is the best idea.

[MaxDesiatov]

@portellaa thanks for the write up and the contribution! I'll keep this open for a few days for anyone interested to post their feedback, otherwise I'll merge it if no strong reasons against it are found by that time 🙂

[portellaa]

no worries, in the meanwhile i will try to found a time where i can work on a solution to integrate xmlns properly, with validation and parsing 🙂

more information regarding xsd and xsi https://www.w3.org/TR/xmlschema11-1/#xsd-namespace and https://www.w3.org/TR/xmlschema11-1/#xsi-namespace

i will keep this here to use later as a guidance in the implementation 😊

[MaxDesiatov]

Please also have a look at the existing shouldProcessNamespaces property on XMLDecoder. Maybe there's a way to make the API "symmetric" for both decoders and encoders, where XMLDecoder would able to figure out the namespace and strip off prefixes if needed, while XMLEncoder could take a namespace as an argument and attach corresponding prefixes. Then shouldProcessNamespaces could be deprecated after it's all made "symmetric".

Again, this needs some design and research work, so if you would like to avoid that, we could keep the current shouldProcessNamespaces, merge this PR adding rootAttributes and leave it at that as a stop gap. Which would be also fine given that XML is more of a niche thing at this point as you said, and I haven't seen any demand for proper XML namespace handling yet 🙂

[portellaa]

I did the pipelines failed? they don't show info about what happened. it seems they didn't even run. I only did a squash of my commits and had to force push. 😕

[MaxDesiatov]

Thanks, seems like there was an intermitten problem with GitHub Actions, I've restarted the jobs, the Danger job is still failing, but it's not critical in any way.

BTW, in this project and most of the projects that I maintain there's no need to squash and/or force push, PRs can contain whatever makes sense and easier to manage for the purpose of a PR, including merge commits. They are always merged with "Squash and merge" after that to maintain linear history in master and a reference to original PRs via id in their commit message, so all merge commits will be gone as squash is performed automatically anyway.

[portellaa]

it's an habit sorry. the merge strategy it's not visible so i prefer to always squash, but i will have in mind that i don't need to do it on your repos 😃

[MaxDesiatov]

This will be released as a part of 0.11, thanks again @portellaa!