add errors to the record if ApiException is raised

[timdiggins]

This does a very basic fix to #538 by parsing the raised ApiException.

This won't help if an ApiException isn't thrown (unexpected error).
It also feels rather ugly having to catch and re-raise, but any other
change would be a very significant refactor.

[timdiggins]

Why are you re-raising the captured exception in the non bang method? If people want exceptions, they should use save!

Perhaps raise unless e.validation_errors could be the first line of the exception block?

I also don't think that you need to call valid? in the exception handler as it has already been called in save!. I think your test stub of valid? is preventing that from being called, so the errors object is never configured. Change the way you're stubbing the object or configure a test object that is valid to avoid the requirement to stub altogether.

@toxaq Thanks for the review. Good catch on the valid? it's not needed at all in fact (I had dropped the unnecessary stub of valid? but didn't drop the implementation 🤦 ) and I've dropped it from the PR.

I agree that the catch and reraise is a bit smelly -- there could be an underlying method that returns true or false or raises an exception -- save! can then raise an exception if it's false and save can return its value but catch an exception and return true.

I think this test case has other flaws:

• needs a case where there is another XeroizerError (not an ApiException) raised
• needs a case where there is a ApiException raised but there are no non-empty ValidationError elements in the response (this seems to happen quite a bit in the wild)
• the xml fixture is out of date (Xero no longer seems to return xml declarations with bogus UTF-16 encoding marking - it seems to leave off the xml declaration altogether).
• I don't think it should be stubbing the test target - that is the biggest smell of all. Maybe stubbing the http request/response would be better -- either with WebMock or my preference would be with VCR (which is good as it would be easy to then compare fixtures against real life xero responses and refresh them easily)

Thoughts on the above welcome -- if I have time I'll try to illustrate one or other approach in the PR I've tried extracting the code into a third method -- see what you think

[rjaus]

Not directly related: But where did @toxaq's feedback come from? Either way, appreciated.

I tried this out late last week, and I ran into a few things.

The below response results in two validators errors being parsed out under invoice.errors, tho they were both associated with :base. Now this is to be expected, based on the response.

<ApiException xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
  <ErrorNumber>10</ErrorNumber>
  <Type>ValidationException</Type>
  <Message>A validation exception occurred</Message>
  <Elements>
    <DataContractBase xsi:type="Invoice">
      <ValidationErrors>
        <ValidationError>
          <Message>Email address must be valid.</Message>
        </ValidationError>
      </ValidationErrors>
      <Contact>
        <ValidationErrors>
          <ValidationError>
            <Message>Email address must be valid.</Message>
          </ValidationError>
        </ValidationErrors>
        <ContactID>24989a56-2f1a-4037-ba13-28eb910fcfbc</ContactID>
        <ContactStatus>ACTIVE</ContactStatus>
        <Name>Test</Name>
        <EmailAddress>asdf@ASDF</EmailAddress>
        <Addresses>
          <Address>
            <AddressType>STREET</AddressType>
          </Address>
          <Address>
            <AddressType>POBOX</AddressType>
          </Address>
        </Addresses>
        <Phones>
          <Phone>
            <PhoneType>DEFAULT</PhoneType>
          </Phone>
          <Phone>
            <PhoneType>DDI</PhoneType>
          </Phone>
          <Phone>
            <PhoneType>FAX</PhoneType>
          </Phone>
          <Phone>
            <PhoneType>MOBILE</PhoneType>
          </Phone>
        </Phones>
        <UpdatedDateUTC>2021-10-19T23:21:01.4136751Z</UpdatedDateUTC>
        <IsSupplier>false</IsSupplier>
        <IsCustomer>true</IsCustomer>
      </Contact>
      <Date>2021-10-20T00:00:00</Date>
      <DueDate>2021-10-30T00:00:00</DueDate>
      <Status>DRAFT</Status>
      <LineAmountTypes>Exclusive</LineAmountTypes>
      <LineItems>
        <LineItem>
          <LineItemID>45154980-13be-4dce-b626-48ef7873a141</LineItemID>
        </LineItem>
      </LineItems>
      <SubTotal>0.00</SubTotal>
      <TotalTax>0.00</TotalTax>
      <Total>0.00</Total>
      <CurrencyCode>AUD</CurrencyCode>
      <Type>ACCREC</Type>
      <InvoiceID>00000000-0000-0000-0000-000000000000</InvoiceID>
      <InvoiceNumber>ORC1102</InvoiceNumber>
      <AmountDue>0.00</AmountDue>
      <AmountPaid>0.00</AmountPaid>
      <SentToContact>false</SentToContact>
      <CurrencyRate>1.0000000000</CurrencyRate>
      <HasErrors>true</HasErrors>
    </DataContractBase>
  </Elements>
</ApiException>

At present it appears most of the validation handling is being performed in the models, prior to sending a request to the API.

So I guess the question is:

1. Do we continue validating client side, in the models?
2. Do we start parsing validation errors from the response, and attributing them to their corresponding entity?
3. Or both?

It seems at present it's a bit of both, but it's not entirely consistent.

[toxaq]

Not directly related: But where did @toxaq's feedback come from? Either way, appreciated.

Here, timdiggins@f5908ba, although it took some effort to find. I'm not fluent in Github's UI.

1. Debatable, I was surprised that validation had been implemented gem side, as you end up with double validation in regards to these sorts of situations. I would prefer it to be Xero side only.
2. Yes. A library should decorate the errors for consumability. Hiding them is the worst thing we can do
3. As above 🤣

[timdiggins]

In regards to the validation happening client side or just leave to server-side...

The concept behind the xeroizer library seems to be that the models behaves analogously to Rails models/records and that the Xero API is like a db (serialization). In that worldview the Xeroizer validations are attempting to prevent db-level errors from occurring. In which case (arguably) we're wrong to expect the Xero api errors to be surfaced as validation errors, and instead should be thrown as errors (which the client can the extract meaning from).

But the problem with this is that Xeroizer is always going to be playing catch up with the Xero api, and it's quite hard to keep them up to date.

Equally, I have found that there are still cases where Xero is rejecting the request with a 4xx and there are no ValidationErrors present in the response. This is really unhelpful but means we can't rely on Xero's api to be provide meaningful responses in this way. I think at this point it would be useful to have the entire response to log in an exception tracker (bugsnag, sentry and so on) rather than swallow the error. In which case the .save is not very helpful and really it would be better to just have .save! and force people to deal with the error.

Perhaps the way to do this is to only allow .save to swallow the error (and return false) if there is a meaningful ValidationError to pass on, otherwise it should raise the (lower-level) error (including the response body). This is a bit of a weird contract for the .save method, but seems the most useful to me as a consumer of this library.

[nnc]

Perhaps the way to do this is to only allow .save to swallow the error (and return false) if there is a meaningful ValidationError to pass on, otherwise it should raise the (lower-level) error (including the response body). This is a bit of a weird contract for the .save method, but seems the most useful to me as a consumer of this library.

After many years of using this library and having to dig through its guts more than I'd like to be able to deal with such errors from the API, I would 100% agree with this sentiment.
But given this would be breaking the Ruby convention re bang and non-bang methods, it would need to be documented quite clearly that #save will raise errors as well. And adding an example to readme that shows how to deal with errors that include raw response body from Xero API will go a long way.

Thanks for working on this!

[toxaq]

In which case the .save is not very helpful and really it would be better to just have .save! and force people to deal with the error.

Indeed, this was a design decision made in 2.18 (or around there) to change the save method from throwing exceptions and adding a bang method that did. Previously save exactly threw exceptions.

I find Xeroizer validations completely unhelpful. I'm already working with models that are validated, so I just want to send that data to Xero unmolested and have a rubyish way of reading the response to that. Clearly others may feel differently so I don't mind that they stay.

To me it's clear that save should populate error messages and save! should throw exceptions.

[rjaus]

To me it's clear that save should populate error messages and save! should throw exceptions.
Agreed

I feel the only detail to clarity here is how we want validation errors raised and populated.

As I mentioned above: #551 (comment)

If validation isn't occurring in the model, then to associate an validation with an entity requires parsing the validation errors from the response and classifying it.

By default, all validation errors are classified as :base which seems less than helpful. As with the example shown, the same error appears twice, as the error is within a nested entity.

The API also supports sending multiple invoices/entities in a single request, so it may not be clear which entity the validation error relates to (need to verify this).

So if we can agree what they should look like, we can move forward with a change.

I find Xeroizer validations completely unhelpful. I'm already working with models that are validated, so I just want to send that data to Xero unmolested and have a rubyish way of reading the response to that. Clearly others may feel differently so I don't mind that they stay

Fair enough. I don't have any further clarity on this issue. If others felt the same we could remove model validation in the next major version, but we'd need more feedback from the community. Are model validations not skippable in their current form?

[toxaq]

Just to clarify, my statement on validation usefulness was just merely anecdata from my point of consumption and was not intended as direction for others to take or implement.

In regards to how the validation errors should be populated I have no problem with them going against :base. My use case is in a background job and I simply create a report of why invoices failed to be recorded so they're accumulated regardless (and hence why the local validation is not utilised). Customer records are connected independently first, so there's no confusion around those as children.

I've not used the bulk invoice option, so I don't have any visibility on how best that would be decorated back on the Xeroizer models. I haven't looked at the tests, but perhaps we could start a list of error scenarios, so that we can have some tests to outline the intention of the error decoration. This would at least give a starting point on the debate over where they should be.