Gyoku.xml produces wrong results with arrays of hashes

[dredozubov]

It produces:

h = {attrs: [{attr: 1}, {attr: 2}, {attr: 3}]}
=> {:attrs=>[{:attr=>1}, {:attr=>2}, {:attr=>3}]}
Gyoku.xml(h)
=> "<foo><bar>1</bar></foo><foo><bar>2</bar></foo><foo><bar>3</bar></foo>"

I think this behaviour is completely incorrect. I believe it should return: <foo><bar>1</bar><bar>2</bar><bar>3</bar></foo>.
I haven't got time to look it yet, but It may be related to #40

[tjarratt]

Hey @dredozubov I think in your example the output for the given input should be

Gyoku.xml(h) => "<attrs><attr>1</attr></attrs><attrs><attr>2</attr></attrs><attrs><attr>3</attr></attrs>"

In general, I agree with you that Gyoku probably should not serialize an array of hashes like that, but I believe there is a historical reason why it does. Looking back through the repo history, it seems like this has been how this has worked since last 2010, when Gyoku was extracted from Savon. My best guess, given that I wasn't around at that time, is that it was this way because some soap service that Savon supported at the time needed it that way.

Did you file this issue because you're trying to pass an array of hashes as parameters to an operation with Savon? A little more context would help me better understand your problem. Changing this behavior would certainly break tests in Gyoku, and probably implicitly break Savon users (which would be a Bad Thing™ indeed).

Also, I'm pretty sure this is unrelated to #40 -- that bug is just stating that calling Gyoku.xml modifies the hash provided.

[dredozubov]

@tjarratt yeah, you're right about my example, i was typing it and, whatever, you get the point.

Yeah, we were building structurally similar query with savon(v2). This behaviour is obviously wrong and weird for tree-like structures, such as xml. It rendered gyoku _absolutely useless_ to us(we were going to use it with small queries) and we just globally switch to Nokogiri::Builder.

Changing this behaviour should break the tests. It's really hard to imagine why this behaviour could be needed at all. I believe it should be done(as a major version bump, to maintain semantic versioning) and Savon should manage with explicit gemspec dependencies and probably switch to new version with Savon 3 release.

[tjarratt]

Cool, thanks for enlightening me!

I totally agree that a major version bump is in order. Unsure when I'll have time to make this change, possibly later this week?

If you're interested, I'd be more than happy to merge a pull request for this behavior. I think you have a really good idea of what it should do, and I agree completely that outside of the context of Savon, this is pretty crazy and wrong.

[rbbrown]

@tjarrat we also have run smack up against this Gyoku behavior, which we definitely consider undesireable. Just like @dredozubov, we are trying to serialize a hash element that is an array, and finding the key for the entire array wrapped around each element.

On the plus side, finding this issue has relieved us of the need to flail about trying to figure out what we were doing wrong. A fix soon would be deeply appreciated.

[dredozubov]

@tjarratt I think i'll try to look into it later this week and submit a PR. Time is an issue, but i think i will manage.

[dredozubov]

@tjarratt I gave it a quick look and gyoku has quite a lot of quirks. It is somewhat painful to preserve the rest of this complex array behaviour and do this kind of change.

I think you have a really good idea of what it should do

I'm not quite sure we're on the same page on that. Let's talk it out first. :) It's quite silly to expect key attribute for "array of hashes" scenario right there https://github.com/savonrb/gyoku/blob/master/lib/gyoku/array.rb#L13. Minimal scenario - we make it optional and probably do a big rewrite of Gyoku::Array.to_xml and Gyoku::Array.iterate_with_xml.

It elevates other problems to the surface though. I get a very inconsistent feel from current behaviour with arrays and i get even more discouraged, when i see things like this #37 is ridiculous IMO. Why do this tags should be named 'element', what is a nested array in the xml tree?) merged recently. Why do we have stuff like that? Xml is a tree-like structure and we should treat it as such on API level.

I'd gladly proposed to drop support of all input data structures except Arrays of hashes in Gyoku::Array.to_xml. It'll dramatically reduce complexity by delegating all attributes/self-closing and other stuff to Gyoku::Hash.to_xml, eliminate double-edged key argument and the most important - will make an API predictable in the next major version. To make it work, we really need to understand where it is going.

Would like to know what you think. Peace. ✌️