Skip to content

Conversation

@artonge
Copy link
Contributor

@artonge artonge commented Feb 8, 2021

The upgrade to xmlbuilder2 broke some generated XMLs, the metadata is part of them as of AuthNRequest, there is probably more if the XML is build with arrays.

Explanation:

Given this code:

root.ele({
  number: [ 
    "one", 
    "two",
    { three: { "@val": 3 } }
  ]
});

xmlbuilder used to render array like that:

<number>
  <one/>
  <two/>
  <three val="3"/>
</number>

And xmlbuilder2 now renders them like that:

<number>one</number>
<number>two</number>
<number>
  <three val="3"/>
</number>

There was the flag separateArrayItems in xmlbuilder to change the behavior but I do not see it in xmlbuilder2. The only option seems to fix all XMLs generation code if it uses arrays.

The upgrade to `xmlbuilder2` broke some generated XMLs, the metadata is part of them as of [AuthNRequest](#227 (comment)), there is probably more if the XML is build with arrays.

Explanation, given this code:
```
root.ele({
  number: [ 
    "one", 
    "two",
    { three: { "@Val": 3 } }
  ]
});
```

`xmlbuilder` used to [render array like that](https://github.com/oozcitak/xmlbuilder-js/wiki/Conversion-From-Object#arrays):
```
<number>
  <one/>
  <two/>
  <three val="3"/>
</number>
```

And  `xmlbuilder2` now [renders them like that](https://oozcitak.github.io/xmlbuilder2/object-conversion.html):
```
<number>one</number>
<number>two</number>
<number>
  <three val="3"/>
</number>
```

There was the flag `separateArrayItems` in `xmlbuilder` to change the behavior but I do not see it in `xmlbuilder2`.
@artonge artonge changed the title Fix create_metadata output WIP Fix create_metadata output Feb 8, 2021
@artonge artonge changed the title WIP Fix create_metadata output Fix create_metadata output Feb 8, 2021
@mcab mcab self-assigned this Feb 8, 2021
@mcab
Copy link
Member

mcab commented Feb 8, 2021

Ah, yikes! Thanks for the speedy PR; I thought test cases properly covered all of the xmlbuilder uses.

Can we create a case for create_metadata() in tests to capture this regression?

EDIT: working on a test case for this currently.

When we upgraded to xmlbuilder2, the method we were using to create XML
documents slightly changed. See 57b1486 and
c910edb for more context.

This change explicitly checks for the amount of nodes that refer to
SPSSODescriptors. Before we upgraded xmlbuilder, there should only be one. When
we upgraded to xmlbuilder2, it registered 5. With this change to
create_metadata, it is now 1 again.
@artonge
Copy link
Contributor Author

artonge commented Feb 8, 2021

I think the update to xmlbuilder2 also removed some non essential properties, like on the keys nodes. I did not investigate that.

@mcab
Copy link
Member

mcab commented Feb 8, 2021

That's really unfortunate. I'm going to reopen that as an issue to check, and work on that separate from this immediate PR.

If you believe the test case covers the regression, I'd be happy to merge this in.

@artonge
Copy link
Contributor Author

artonge commented Feb 8, 2021

The test case should do it.

I feel like the best would probably be to compare the output with an expected string. But maybe that's for another issue 😉

@mcab mcab merged commit 239bd72 into Clever:master Feb 8, 2021
manicphase pushed a commit to manicphase/peertube-official-plugins that referenced this pull request Jul 27, 2022
…e XML output.

This can be restored when this PR is merged: Clever/saml2#229
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 this pull request may close these issues.

2 participants