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

Fix "undefined method `[]' for #<Nori::StringIOFile>" when adding File #495

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

cgunther
Copy link
Contributor

When calling File#add, the response XML sets a type="file" attribute
on the baseRef element, where "file" corresponds to the NetSuite
record. This then causes Nori (the XML parser used by Savon, the SOAP
client) to interpret that element as representing a base64 encoded file,
so it tries to get fancy about how it parses that element into a hash by
returning an instance of it's StringIOFile class:
https://github.com/savonrb/nori/blob/f59f8e18b0e53285c87d75a635058745e66b0bfb/lib/nori/xml_utility_node.rb#L131-L136

Either NetSuite's doing something non-standard with it's use of the
type attribute that Nori is trying to enforce, or Nori is
over-aggressive in trying to typecast to aid the developer.

The end result was that when we tried to extract the internal_id from
the response, the body was actually an instance of StringIOFile, not
a hash:

@internal_id = response.body[:@internal_id]

To work around this, as I don't see a way to disable such behavior in
Savon/Nori, if we detect the baseRef element was parsed to
StringIOFile, we'll then take the raw XML and parse out the baseRef
ourselves, returning a hash with the internal_id as we'd exect from
non-File responses.

I'm not thrilled with this solution. If we ever needed something else
from the baseRef element, this effectively drops all other attributes
for file records. It also introduces explicit references to Nori and
Nokogiri, both of which are dependencies of Savon, but I'm not sure if
that means this gem should list them as explicit dependencies to guard
against Savon replacing them in a future update. Listing them as
dependencies would then require keeping their version constraints in
sync with Savon most likely.

I believe this answers a question from #481:
#481 (comment)

However the fix in #481 solves it by not trying to extract the
internal_id, which would create a problem if someone wanted to add a
file then save the ID in their own database for future use.

When calling `File#add`, the response XML sets a `type="file"` attribute
on the `baseRef` element, where "file" corresponds to the NetSuite
record. This then causes Nori (the XML parser used by Savon, the SOAP
client) to interpret that element as representing a base64 encoded file,
so it tries to get fancy about how it parses that element into a hash by
returning an instance of it's `StringIOFile` class:
https://github.com/savonrb/nori/blob/f59f8e18b0e53285c87d75a635058745e66b0bfb/lib/nori/xml_utility_node.rb#L131-L136

Either NetSuite's doing something non-standard with it's use of the
`type` attribute that Nori is trying to enforce, or Nori is
over-aggressive in trying to typecast to aid the developer.

The end result was that when we tried to extract the `internal_id` from
the response, the `body` was actually an instance of `StringIOFile`, not
a hash:
https://github.com/NetSweet/netsuite/blob/f0e46a076d0e7cb2abd5e9001ccbfd4bbb3d35c3/lib/netsuite/actions/add.rb#L80

To work around this, as I don't see a way to disable such behavior in
Savon/Nori, if we detect the `baseRef` element was parsed to
`StringIOFile`, we'll then take the raw XML and parse out the `baseRef`
ourselves, returning a hash with the `internal_id` as we'd exect from
non-`File` responses.

I'm not thrilled with this solution. If we ever needed something else
from the `baseRef` element, this effectively drops all other attributes
for file records. It also introduces explicit references to `Nori` and
`Nokogiri`, both of which are dependencies of Savon, but I'm not sure if
that means this gem should list them as explicit dependencies to guard
against Savon replacing them in a future update. Listing them as
dependencies would then require keeping their version constraints in
sync with Savon most likely.

I believe this answers a question from NetSweet#481:
NetSweet#481 (comment)

However the fix in NetSweet#481 solves it by not trying to extract the
`internal_id`, which would create a problem if someone wanted to add a
file then save the ID in their own database for future use.
@cgunther cgunther force-pushed the fix-add-file-failing branch from 6abf4ba to 827c4e0 Compare December 6, 2021 20:37
@iloveitaly
Copy link
Member

Either NetSuite's doing something non-standard with it's use of the
type attribute that Nori is trying to enforce

My guess is NetSuite is doing something non-standard. I've seen this in other places (the wsdl url).

I'm not thrilled with this solution. If we ever needed something else
from the baseRef element

Thanks for explaning this tradeoff. I think it's a good approach given the situation.

It also introduces explicit references to Nori and
Nokogiri, both of which are dependencies of Savon, but I'm not sure if
that means this gem should list them as explicit dependencies to guard
against Savon replacing them in a future update.

Given that nori is part of the savon GH org, and nokogiri is a very core part of the ruby XML parsing ecosystem, I think it's safe to not include these an optional dependencies for now. I think there is a very small risk that this breaks us in the future.

@iloveitaly iloveitaly merged commit f502ac3 into NetSweet:master Dec 14, 2021
@cgunther cgunther deleted the fix-add-file-failing branch December 14, 2021 17:31
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