Add support for root attributes propagation #160

portellaa · 2020-03-15T14:52:33Z

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?

Sources/XMLCoder/Encoder/XMLEncoder.swift

MaxDesiatov · 2020-03-17T22:18:28Z

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 · 2020-03-18T17:23:10Z

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 · 2020-03-18T18:27:20Z

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 · 2020-03-18T19:14:21Z

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 · 2020-03-18T19:41:35Z

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 · 2020-03-18T20:47:38Z

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 · 2020-04-09T00:28:36Z

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%)`	⬇️
Sources/XMLCoder/Auxiliaries/XMLCoderElement.swift	`95.56% <77.77%> (+0.04%)`	⬆️
Sources/XMLCoder/Auxiliaries/KeyedStorage.swift	`95.55% <0.00%> (+0.20%)`	⬆️
...s/XMLCoder/Encoder/XMLKeyedEncodingContainer.swift	`74.01% <0.00%> (+2.36%)`	⬆️

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 · 2020-04-09T00:36:23Z

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 · 2020-04-09T14:21:12Z

Tests/XMLCoderTests/RootLevetExtraAttributesTests.swift

+import XCTest
+@testable import XMLCoder
+
+class RootLevetExtraAttributesTests: XCTestCase {


Suggested change

class RootLevetExtraAttributesTests: XCTestCase {

final class RootLevetExtraAttributesTests: XCTestCase {

i completely agree with this, but i checked the other classes and so i followed the same. changed 😊 👍

Sorry about that, the library does have some inconsistencies which is caused more by the lack of my attention than anything else 🙈

that's normal 🤷 i should have followed what i usually do, but i prefer to respect the "style" 😅

MaxDesiatov · 2020-04-09T14:24:29Z

Sources/XMLCoder/Encoder/XMLEncoder.swift

@@ -318,28 +322,35 @@ open class XMLEncoder {
    ///
    /// - parameter value: The value to encode.
    /// - parameter withRootKey: the key used to wrap the encoded values.
+    /// - parameter rootAttributes: the list of attributes to be added to root node


Suggested change

/// - parameter rootAttributes: the list of attributes to be added to root node

/// - parameter rootAttributes: the list of attributes to be added to the root node

ups thanks 🙈

MaxDesiatov

Apart from minor nitpicks, the code looks good to me as such, thank you for adding the tests! I have to say, I'm still a bit ambiguous about the API. Given that the main use case is in handling xmlns attributes, I wonder if there's a better way to do this, maybe more focused on xmlns functionality itself in conjunction with how namespace prefixes are handled, but I don't have enough experience with handling XML namespaces to say what API would work better.

I'd be happy to hear if frequent contributors and users of downstream dependencies of XMLCoder have any opinions (cc @bwetherfield)

MaxDesiatov · 2020-04-09T14:39:51Z

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 · 2020-04-09T15:58:15Z

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 · 2020-04-09T16:00:55Z

@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 · 2020-04-09T16:06:39Z

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 · 2020-04-09T16:20:37Z

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 🙂

bwetherfield

Overall, LGTM! Just saw a few possible redundancies.

bwetherfield · 2020-04-10T03:19:32Z

Sources/XMLCoder/Auxiliaries/XMLCoderElement.swift

@@ -348,30 +350,30 @@ extension XMLCoderElement {
        self.init(key: key, elements: elements, attributes: attributes)
    }

-    init(key: String, box: SimpleBox) {
+    init(key: String, box: SimpleBox, attributes: [Attribute] = []) {


I think you can do without the attributes in the SimpleBox initializer!

yes yes nice catch 👍

bwetherfield · 2020-04-10T03:22:52Z

Sources/XMLCoder/Encoder/XMLEncoder.swift

+                       rootAttributes: rootAttributes,
                       userInfo: userInfo)
    }

+    public var rootAttributes: [String: String] = [:]
+


Are these still needed?

no we don't. thank you 😊

bwetherfield · 2020-04-10T03:23:50Z

Sources/XMLCoder/Encoder/XMLEncoder.swift

@@ -293,6 +293,7 @@ open class XMLEncoder {
        let keyEncodingStrategy: KeyEncodingStrategy
        let nodeEncodingStrategy: NodeEncodingStrategy
        let stringEncodingStrategy: StringEncodingStrategy
+        let rootAttributes: [String: String]


thank you for notice 🙏

portellaa · 2020-04-12T00:59:22Z

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 · 2020-04-12T10:25:24Z

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 · 2020-04-13T09:27:17Z

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 · 2020-04-13T10:54:50Z

This will be released as a part of 0.11, thanks again @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.

houndci-bot reviewed Mar 15, 2020

View reviewed changes

Sources/XMLCoder/Encoder/XMLEncoder.swift Outdated Show resolved Hide resolved

houndci-bot reviewed Mar 15, 2020

View reviewed changes

Sources/XMLCoder/Encoder/XMLEncoder.swift Outdated Show resolved Hide resolved

MaxDesiatov added the test case missing There is not test case that covers the change label Mar 17, 2020

portellaa force-pushed the feat/root_attributes branch from 5514a60 to d24ff75 Compare April 9, 2020 00:27

portellaa force-pushed the feat/root_attributes branch from d24ff75 to cc14da0 Compare April 9, 2020 00:35

portellaa force-pushed the feat/root_attributes branch 2 times, most recently from 8ed6599 to 65c6707 Compare April 9, 2020 13:09

MaxDesiatov reviewed Apr 9, 2020

View reviewed changes

bwetherfield reviewed Apr 10, 2020

View reviewed changes

feat: support root attributes propagation

1b7cb2a

portellaa force-pushed the feat/root_attributes branch from 3a126c8 to 1b7cb2a Compare April 11, 2020 13:57

MaxDesiatov removed the test case missing There is not test case that covers the change label Apr 13, 2020

MaxDesiatov added the enhancement New feature or request label Apr 13, 2020

MaxDesiatov merged commit d57b8f2 into CoreOffice:master Apr 13, 2020

MaxDesiatov mentioned this pull request Apr 13, 2020

Add support for TopLevelEncoder in XMLEncoder #174

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for root attributes propagation #160

Add support for root attributes propagation #160

portellaa commented Mar 15, 2020 •

edited by MaxDesiatov

MaxDesiatov commented Mar 17, 2020

portellaa commented Mar 18, 2020 •

edited

MaxDesiatov commented Mar 18, 2020

portellaa commented Mar 18, 2020

portellaa commented Mar 18, 2020

MaxDesiatov commented Mar 18, 2020

codecov bot commented Apr 9, 2020 •

edited

portellaa commented Apr 9, 2020

MaxDesiatov Apr 9, 2020

portellaa Apr 9, 2020

MaxDesiatov Apr 9, 2020

portellaa Apr 12, 2020

MaxDesiatov Apr 9, 2020

portellaa Apr 9, 2020

MaxDesiatov left a comment

MaxDesiatov commented Apr 9, 2020 •

edited

portellaa commented Apr 9, 2020

MaxDesiatov commented Apr 9, 2020

portellaa commented Apr 9, 2020

MaxDesiatov commented Apr 9, 2020

bwetherfield left a comment

bwetherfield Apr 10, 2020

portellaa Apr 11, 2020

bwetherfield Apr 10, 2020

portellaa Apr 11, 2020

bwetherfield Apr 10, 2020

portellaa Apr 11, 2020

portellaa commented Apr 12, 2020

MaxDesiatov commented Apr 12, 2020 •

edited

portellaa commented Apr 13, 2020

MaxDesiatov commented Apr 13, 2020

	class RootLevetExtraAttributesTests: XCTestCase {
	final class RootLevetExtraAttributesTests: XCTestCase {

	/// - parameter rootAttributes: the list of attributes to be added to root node
	/// - parameter rootAttributes: the list of attributes to be added to the root node

Add support for root attributes propagation #160

Add support for root attributes propagation #160

Conversation

portellaa commented Mar 15, 2020 • edited by MaxDesiatov

MaxDesiatov commented Mar 17, 2020

portellaa commented Mar 18, 2020 • edited

MaxDesiatov commented Mar 18, 2020

portellaa commented Mar 18, 2020

portellaa commented Mar 18, 2020

MaxDesiatov commented Mar 18, 2020

codecov bot commented Apr 9, 2020 • edited

Codecov Report

portellaa commented Apr 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxDesiatov left a comment

Choose a reason for hiding this comment

MaxDesiatov commented Apr 9, 2020 • edited

portellaa commented Apr 9, 2020

MaxDesiatov commented Apr 9, 2020

portellaa commented Apr 9, 2020

MaxDesiatov commented Apr 9, 2020

bwetherfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

portellaa commented Apr 12, 2020

MaxDesiatov commented Apr 12, 2020 • edited

portellaa commented Apr 13, 2020

MaxDesiatov commented Apr 13, 2020

portellaa commented Mar 15, 2020 •

edited by MaxDesiatov

portellaa commented Mar 18, 2020 •

edited

codecov bot commented Apr 9, 2020 •

edited

MaxDesiatov commented Apr 9, 2020 •

edited

MaxDesiatov commented Apr 12, 2020 •

edited