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

Action get_all return errors #423

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

freezepl
Copy link
Contributor

@freezepl freezepl commented Jan 4, 2019

Add ability to return errors from get_all command.

To preserve compatibility with current approach errors are added to the class method.

[3] pry(main)> currency = NetSuite::Records::Currency
=> NetSuite::Records::Currency < Object
[5] pry(main)> currencies = currency.get_all(insufficient_perms.netsuite_credentials)
...
</soapenv:Envelope>

=> false
[6] pry(main)> currency.errors
=> [
    [0] #<NetSuite::Error:0x00007fd66e821d28 @type="ERROR", @code="INSUFFICIENT_PERMISSION", @message="Permission Violation: You need  the 'Lists -> Currency' permission to access this page. Please contact your account administrator.">
]
[7] pry(main)> currencies
=> false

My first approach was to return NetSuite::Response object which would look like this:

=> #<NetSuite::Response:0x00007f8d960c8548 @success=false, @header=nil, @body=nil, @errors=[#<NetSuite::Error:0x00007f8d960c8598 @type="ERROR", @code="INSUFFICIENT_PERMISSION", @message="Permission Violation: You need  the 'Lists -> Currency' permission to access this page. Please contact your account administrator.">]>

@freezepl
Copy link
Contributor Author

freezepl commented Jan 7, 2019

I found #405 this PR which imo is better solution. We could add there support for invalid account creds there.
@iloveitaly what do you think?

@iloveitaly iloveitaly force-pushed the master branch 2 times, most recently from 7185ffc to df796ee Compare May 28, 2019 15:55
@cgunther
Copy link
Contributor

I think there's still value in this PR separately from #405 to handle any kind of error returned, not just insufficient permission. I also think this would be valuable to extend to any action using class methods (ie. delete_list, get_deleted, get_list, search, update_list, upsert_list), as they all return false on failure, but don't offer a way to dig into why it failed.

For example, I just hit a related issue using search where I was using the anyOf operator and passed more than 1,000 values, which NetSuite didn't like. It'd be great if there was access to the error after search, just like what you did for get_all.

For reference, here's the response XML from my search that includes the error:

<?xml version="1.0" encoding="UTF-8"?>
<soapenv:Envelope xmlns:soapenv="http://schemas.xmlsoap.org/soap/envelope/" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <soapenv:Header>
    <platformMsgs:documentInfo xmlns:platformMsgs="urn:messages_2020_2.platform.webservices.netsuite.com">
      <platformMsgs:nsId><!-- SNIP --></platformMsgs:nsId>
    </platformMsgs:documentInfo>
  </soapenv:Header>
  <soapenv:Body>
    <searchResponse xmlns="urn:messages_2020_2.platform.webservices.netsuite.com">
      <platformCore:searchResult xmlns:platformCore="urn:core_2020_2.platform.webservices.netsuite.com">
        <platformCore:status isSuccess="false">
          <platformCore:statusDetail type="ERROR">
            <platformCore:code>MAX_VALUES_EXCEEDED</platformCore:code>
            <platformCore:message>Too many values specified for a multi-select field, the maximum is 1000</platformCore:message>
          </platformCore:statusDetail>
        </platformCore:status>
      </platformCore:searchResult>
    </searchResponse>
  </soapenv:Body>
</soapenv:Envelope>

@iloveitaly
Copy link
Member

We definitely need to improve how this is handled right now, but I don't like the idea of storing the results in global state. I've found that global state will nearly always come back to bite you in strange and hard-to-debug ways. In this PR, the error state is not cleared unless another call is made and would persist indefinitely. In multi-threaded environments this would be very problematic.

I'm fine with a breaking change here if it creates a better UX for the future. In addition to get_all we need to handle this same case for the search action as well.

https://github.com/NetSweet/netsuite/pull/405/files handles a different case, but I think a similar approach could be better here:

  • Create a new exception type which includes an errors array
  • Throw the new exception type when errors are encountered

This eliminates the risk of global state, although I don't like the idea of using exceptions to manage control flow. Returning completely different types of objects is worse though and would require ugly-looking code to handle a (hopefully) rare case where the NS call fails.

@cgunther @freezepl what do you think? Curious to hear about pros/cons I'm not thinking about here.

@cgunther
Copy link
Contributor

Speaking mainly from the search scenario I provided above, exceptions would be fine. In my case, I'm throwing an exception anyway when the search returns false, so having the gem throw it itself, plus with details of the exact error, would save me a step of having to check for a false return value.

I think #405 could be a good basis, potentially having a generic error class, then on a case-by-case basis introduce more specific classes (ie. PermissionError) if they might be handled differently on the application side.

I also think #405 does a better job of addressing the problem at the response, rather than in the action class method, which might require repeating the handling for each action implementing a class method.

@iloveitaly
Copy link
Member

@cgunther I agree. I really like the structure of #405 and think we should use that approach for improved error handling going forward.

@Timothyjb What do you think of the approach laid out in #405? Any further thoughts to add to this discussion?

@freezepl anything to add here?

@freezepl
Copy link
Contributor Author

freezepl commented Sep 9, 2021

@iloveitaly I am fine with closing my PR, #405 is open since 2018 and mine is since 2019 and problem is causing a lot of problems for us and for newcomers .

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.

None yet

3 participants