Return parsed response when only failing validation

[JohnStarich]

Hey again Aaron! Let me know your thoughts on this more flexible version of ofxgo.ParseResponse. It still returns an error if things are not parseable, but attempts to continue decoding if it's only hitting Valid()-ation errors. This way I can write institution-specific handling of validation errors and still use the response object.

I ran into this issue with Ally Bank where their severity in the transaction list is not uppercase like the OFX 102 spec requires. I've contacted them, but I don't expect they'll actually change anything since "it works for Quicken". 😉

[aclindsa]

This seems like a reasonable interface change to me, and one which appears to remain backwards-compatible at that! Thank you, again, for your quality PR.

This way I can write institution-specific handling of validation errors and still use the response object.

Are you planning to do that externally to ofxgo, or is that a planned follow-on to this PR? I ask because this current PR mostly allows you to ignore validation wholesale based on the institution, but seems like it would still be difficult to say "For Ally, allow 'Info' instead of 'INFO' but still validate everything else". Perhaps you could rely on whitelisting particular error messages, though that seems a little brittle.

Though, to be honest, ofxgo's current validation is not particularly comprehensive so I'm not sure you'd be losing a ton of utility by ignoring it completely... I always meant to come back and improve it, but once stuff was working well enough, I moved on to other things and lost motivation.

I ran into this issue with Ally Bank where their severity in the transaction list is not uppercase like the OFX 102 spec requires. I've contacted them, but I don't expect they'll actually change anything since "it works for Quicken".

Groan. My guess is that you'll be lucky to get in contact with anyone who even understands what you want changed or why it matters, much less has the motivation to do it. Good luck!

Two additional thoughts as I step back from reviewing your code to thinking about this problem more generally:

1. I don't really think anyone cares if things are not correctly capitalized here. A quick-n-dirty fix would be to add a more permissive mode to validation.
2. Or, what if we found a way to separate the validation from the parsing, or give different severity to different errors? It seems like there are several different reasons ParseResponse/decodeMessageSet may fail, some of which are very minor (Info vs. INFO), and some of which may be more major (completely garbled or incomplete response which isn't going to produce anything useful and may cause unexpected behavior for a program assuming a certain structure to the response). It is reasonable for someone using this library to want to differentiate between errors which mean everything is garbage and those which mean something isn't capitalized...

[JohnStarich]

Are you planning to do that externally to ofxgo, or is that a planned follow-on to this PR?

I think I'm okay with a change like this for now! It seems each institution I've tried direct connect with supports the spec pretty darn well, whereas "web connect" only institutions like Ally try to cut corners. If I run into an institution in the future with poorly supported direct connect, then I may have to come up with another PR like that 👍

My guess is that you'll be lucky to get in contact with anyone who even understands what you want changed or why it matters, much less has the motivation to do it.

Haha I hear that. I had a nice shouting-into-the-void session the other day with Discover when they shut off their direct connect service permanently because "it wasn't secure enough". Ha, sure, OFX 2.2 totally doesn't support an industry standard like OAuth or anything.. 😏

Two additional thoughts

Good points for sure. I thought about making the validation more permissive, then I figured it may require tuning each validator and I might still run into another institution doing crazy shenanigans I didn't anticipate. I do like (2), separating the parse and validate steps would certainly help a ton if I needed to tune an institution a little differently. I was worried this would be heavy-handed and could break backward compatibility, but if you're game then I see no reason not to!

One way we could keep compatibility is to separate top-level funcs to choose which steps are run in custom client code. This could make the library's API a bit messy though. There might be a nicer way to expose these:

func ParseResponse(reader io.Reader) (*Response, error) {
    resp, err := Decode(reader)
    if err != nil {
        return nil, err
    }
    return resp, resp.Valid()
}

func Decode(reader io.Reader) (*Response, error) {
    // ...
}


resp, err := ParseResponse(body) // decodes and validates

resp, err := Decode(body) // only decodes, validation can be run separately

If we decide to go the separate parse (decode) and validate steps, are there any validation errors I missed that aren't critical to parsing?

[JohnStarich]

On a totally unrelated note, would you like me to add something like Travis CI to run some linters and the tests? My concern is only that I'll push broken code by mistake and we merge 😆 (can probably move that to it's own PR or issue to discuss)

Edit: Oops, forgot travis was set up once upon a time. Seems to be ignoring my PR... interesting!

[aclindsa]

Hmm, not sure why travis integration isn't fully working. It does appear to be building your PR, though: https://travis-ci.org/github/aclindsa/ofxgo/builds/666664412

Update: Seems like it could be an issue with travis: https://www.traviscistatus.com/incidents/rx6fhs3wqcln

[aclindsa]

I do like (2), separating the parse and validate steps would certainly help a ton if I needed to tune an institution a little differently. I was worried this would be heavy-handed and could break backward compatibility, but if you're game then I see no reason not to!

One way we could keep compatibility is to separate top-level funcs to choose which steps are run in custom client code. This could make the library's API a bit messy though.

I would like to avoid breaking backwards-compatibility if we can. I don't think there are a ton of users, but I know there are a few. And yes, I was thinking about essentially the same separation you proposed - leaving ParseResponse intact, while adding a non-validating parse method. I think my biggest gripe with this is not being able to revise the naming scheme while adding new methods, but I can get over that.

I take it from how you wrote your example ParseResponse code that you were also thinking of adding a top-level Valid() function to the response objects, also? I was going to propose adding one before noticing that detail.

Do you want to do this as a separate PR, or tack it on to this one? Doesn't matter to me either way.

If we decide to go the separate parse (decode) and validate steps, are there any validation errors I missed that aren't critical to parsing?

Not that I see.

[JohnStarich]

Ok! Makes sense, I think we can avoid breaking things pretty easily by composing func's like with separate Decode and Valid funcs.

Does this sound like a good preliminary design?

func ParseResponse(reader io.Reader) (*Response, error) { ... }

func (r *Response) Valid() (bool, error) { ... }

func UnmarshalResponse(data []byte) (*Response, error) { ... }

I also thought of some alternatives while I was brainstorming:

// Follows the marshal / unmarshal pattern of "encoding/json" and "encoding/xml"
// may not be a great fit for this lib because it's using a strictly internal xml decoder
func (r *Response) UnmarshalXML(d *aclindsaxml.Decoder, start StartElement) error { ... }

var r ofxgo.Response
err := aclindsaxml.Unmarshal(bufBytes, &r)

// Another variant on validating the response with a top-level func,
// though this is an anti-pattern since most `Valid()` checks are receivers (and use `bool, error`)
func ValidResponse(r *Response) error { ... }

Do you want to do this as a separate PR, or tack it on to this one? Doesn't matter to me either way.

Hmm good question. I think a separate PR would be best, if only so I can unblock my other project a little earlier 😄
I just attempted to unexport ErrInvalid so it wouldn't change the API surface area until we get through the next PR, but it seems I can't reference the type in the tests 🤔 I think it's because many of the test files are importing the package they're inside so the only way to access is with the package name ofxgo.errInvalid. Do you mind if I remove those imports from the tests and drop the ofxgo. prefixes from it's references?

[aclindsa]

Does this sound like a good preliminary design?

Yes. I like the initial proposal better than the more-like-go's-xml alternative.

I just attempted to unexport ErrInvalid so it wouldn't change the API surface area until we get through the next PR, but it seems I can't reference the type in the tests thinking I think it's because many of the test files are importing the package they're inside so the only way to access is with the package name ofxgo.errInvalid. Do you mind if I remove those imports from the tests and drop the ofxgo. prefixes from it's references?

I do not mind. I feel like I must've had some reason for doing that in the first place, but I really can't come up with anything good...