Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Empty items handing #35

Closed
dklimkin opened this issue Feb 1, 2013 · 9 comments
Closed

Empty items handing #35

dklimkin opened this issue Feb 1, 2013 · 9 comments
Labels

Comments

@dklimkin
Copy link

dklimkin commented Feb 1, 2013

This issue was first discussed here: savonrb/savon#137

Opening under Nori as it is more relevant.

I'd like to revisit this please. This causes an issue for me, as I can not pass an object returned by the server back to it:

Gyoku.xml(nori.parse(""))
=> "<xml xsi:nil="true"/>"

The server-side accepts empty item, but not a nil. Based on some discussions I believe the empty item should be parsed as an empty string:

Current behavior:
nori.parse("") => {"xml"=>nil}

Expected behavior:
nori.parse("") => {"xml"=>""}

@zgohr
Copy link

zgohr commented Mar 5, 2014

+1

@tjarratt
Copy link
Contributor

tjarratt commented Mar 6, 2014

If I understand the issue correctly, the argument is that a self-closing tag (eg: <xml/>) should be treated the same as a tag with no content (eg: <xml></xml>). If that change was enacted, how would one differentiate between those two cases?

(implicit assumption above, which may not be true: knowing whether something was an empty string or just nil is a valid use case)

I'm willing to suspend disbelief and accept that this would be a very useful change, but I'm also concerned that this would break the API and cause a lot of subtle bugs that would be really annoying for Nori and Savon users to debug.

Is it really too hard to check for nil and replace with an empty string?

@zgohr
Copy link

zgohr commented Mar 6, 2014

Doesn't make much difference to me, I just saw this issue still open so I bumped it. Since it is so easy to handle this edge case on my own, I wouldn't complain if that was the official nori stance. Although it does seem like a default that would be nice to configure via a setting.

@dklimkin
Copy link
Author

dklimkin commented Mar 6, 2014

The problem is, with the current behavior it's not possible to determine is the original XML had this tag included or not. As a result, we can't generate the same XML after parsing it (unless we hardcode the list of all expected fields).

My use case: API returns an object to me, I change one of it's fields and send it back. As a result, I send tags with xsi:nil = true for empty unmodified fields and the request gets rejected (as nils are not allowed).

To respond, it is not hard, when you know which fields were really nil and which were empty, but they are not distinguishable.

@zgohr
Copy link

zgohr commented Mar 6, 2014

That's a good point, but couldn't you just specify at the API level? If you know the API you interact with does not allow xsi:nil="true" then modify a nori setting to parse with your expected behavior.

You're right you wouldn't be able to tell a difference between a nil field and an empty field once parsed, but if the API can't either, I'm not sure I understand the case where it would matter?

@dklimkin
Copy link
Author

dklimkin commented Mar 6, 2014

The API does distinguish them. It omits properties that are nil and sends self-closed tags for the empty ones. It expects the same in the requests. The SOAP API-side is usually a public service implemented by someone else and we have no control over it.

Making this change configurable will definitely solve my issue.

@tjarratt tjarratt added feature and removed feature labels Mar 6, 2014
@tjarratt
Copy link
Contributor

tjarratt commented Mar 6, 2014

@zgohr @dklimkin thanks for discussing the issue in more depth. I hadn't previously considered the case where you wanted to differentiate between </tag> and the absence of the tag; thank you for opening my mind.

I've marked this as a feature for now, implementing this as a user configurable option seems eminently reasonable. Currently, I'm trying to resolve some issues with jruby in savon, so it may be some time before I get around to this. If someone would like to issue a PR for this behavior with tests, I'd love to merge it in.

I'm open to suggestions, but maybe the option could be something like :empty_tags_to_nil and it would default to true?

eg:

require 'nori'
Nori.new(:empty_tags_to_nil => false).parse("<tag/>") # {:tag => ""}

@tjarratt
Copy link
Contributor

tjarratt commented May 7, 2015

Just released the feature that @konstantinn added as Nori v2.6.0. Thank you everyone that helped contribute and continue to improve Nori :)

@nicholaschen
Copy link

Sorry, I don't think the fix that @konstantinn addresses our use case - as it allows you to cast it into an empty string, but doesn't allow for omission of that particular tag (in our case, there's a semantic difference between a value of empty vs. something not being present). Can we re-examine if it's possible to add something that avoids appending nil valued elements in the response entirely?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants