ofxget: Best way to set NEWFILEUID="NONE" in header?

[aclindsa]

I'm hitting an error fetching OFX from a local credit union using ofxget and have found that providing a header like:

<?OFX OFXHEADER="200" VERSION="203" SECURITY="NONE" OLDFILEUID="NONE" NEWFILEUID="904469de-e542-430f-87a1-6551cedbc8b0"?>

leaves me with nothing but a 400 error,

<?OFX OFXHEADER="200" VERSION="203" SECURITY="NONE" OLDFILEUID="NONE" NEWFILEUID="NONE"?>

succeeds!

Notably, in the successful request "NEWFILEUID" is set to "NONE" in the header.

I can of course hack the code locally to do what I want for immediate needs, but:

1. Is this behavior something you would be willing to support in ofxget?
2. If so, how would you prefer I go about adding it?

[csingley]

Well your FI is out of compliance with section 2.2.4 of the spec... but sure, why not?

Have a look in ofxtools.header... specifically ofxtools.header.OFXHeaderV2.__init__() and the ofxtools.header.make_header() helper function, which is called by ofxtools.Client.OFXClient.serialize().

make_header() does the right thing; the problem is that OFXClient.serialize() is hardwired to pass a UUID to that function, without any way to override this behavior. This is a sloppy hack that is now biting you, so it seems like a good time to improve this situation.

I'd say the right way to handle this is to expose the full set of make_header() arguments all the way up the calling stack. That is to say, we need to add oldfileuid and newfileuid arguments to OFXClient.serialize() and OFXClient.download()... I've already added a version argument to these functions, and security is unused by any OFX implementation as far as I'm aware.

As we proceed up the call stack from OFXClient.download() to the functions that call it, e.g. OFXClient.request_statements() or OFXClient.request_accounts()... the existing treatment of the version argument that ultimately gets passed to header.make_header() is to pass the version in as an attribute during instance initialization, and request_statements() grab it from the instance attribute, rather than having version passed in as an argument by the caller. The assumption here is that the appropriate OFX version for a given FI is a constant. However, this won't work for newfileuid and oldfileuid... we can't pass newfileuid into OFXClient.__init__(), use that value when we call OFXClient.request_statements() for a certain set of account options, and then reuse that same value of newfileuid when we call request_statements() for a different set of account options.

If it has a value for NEWFILEUID, each different request must have a unique value. That means we actually need to handle newfileuid and oldfileuid in each of the top-level request functions (request_statements(), request_accounts(), request_profile(), request_tax1099(), and what have you).

I'd think a good first step would be to add to each of these top-level request functions two new args: oldfileuid[Optional[str]] = None, and gen_newfileuid[bool] = True. The oldfileuid could just be passed straight down the calling stack to download(), serialize(), and make_header(). gen_newfileuid, if True, would populate newfileuid from OFXClient.uuid just as it currently does in serialize(); if False, it would pass None for newfileuid all down the function stack.

That would preserve the current behavior by default, but give you something to override. It also provides the beginnings of some hooks in case we later need to implement more of the file error-recovery protocol.

Sound about right?

[csingley]

I implemented the discussed arg in 42aaa1d

It still needs to be brought into a command option for the ofxget script, though, if you need that.

[aclindsa]

Thanks!

I think I actually have multiple issues with this FI - Let me make sure I can get to the bottom of those issues so that I can test everything together and then I'll plan to make a PR for ofxget as well.

[aclindsa]

Okay, I've pushed an attempt at the change to 'ofxget' to add the ability to set NEWFILEUID="NONE". I'm sort of bumbling my way through your library, but I think I'm at least headed in the right direction.

I also have another change in the works for the second issue with my FI (https://github.com/aclindsa/ofxtools/commits/client_set_user_agent), but I'll hold off until the NEWFILEUID one gets through since the two have merge conflicts if they pass each other and I figured it would be easier to consider them separately.

[csingley]

Hopefully the actual library itself isn't too bad (although Client gets a little knotty), but the ofxget script is pretty awful. I'm not good with argparser.

Your NEWFILE PR has got the gist of what needs to happen to ofxget, with a couple caveats:

1. It's not immediately obvious, but it's important that each new option gets registered in DEFAULTS in order for that unpleasant logic about merging configs to work correctly.

2. I think it would be nicer if the --nonewfileuid belonged to one of the argument groups; I guess the format-related one would be most appropriate (i.e. add_format_group(), alongside --unclosedelements and --pretty, which sort of share the same provenance).

These should be pretty minor to implement, would you mind?

Thanks much for updating the tests for your changes

[aclindsa]

It's not immediately obvious, but it's important that each new option gets registered in DEFAULTS in order for that unpleasant logic about merging configs to work correctly.

I believe I've already done this (though it wasn't done when I first posted - I updated my branch in the meantime). Let me know if I didn't do that properly.

I think it would be nicer if the --nonewfileuid belonged to one of the argument groups; I guess the format-related one would be most appropriate (i.e. add_format_group(), alongside --unclosedelements and --pretty, which sort of share the same provenance).

This means it will not get set for 'scan'. Is that okay?

[csingley]

I believe I've already done this (though it wasn't done when I first posted - I updated my branch in the meantime). Let me know if I didn't do that properly.

Looks good, sorry. I was considering a stale version.

This means it will not get set for 'scan'. Is that okay?

Well unclosedelements and pretty are major elements of the scanning function. If we wish to include testing nonewfileuid in the scan, it would make sense for it to be right here.

However mucking around with the scan is a greater undertaking, and probably deserves to be considering as its own separate commit... especially if changes to clientuid are also to be considered as part of the scan.

Thanks.

[aclindsa]

Assuming that you meant:

especially if changes to newfileuid are also to be considered as part of the scan.

...I see your point about this not being an argument to scan, but rather being something varied inside of it.

I'll post a fix momentarily for moving this to the 'format' group.

[aclindsa]

Hopefully the actual library itself isn't too bad

No, it's not bad at all - it's just working through the small stuff that you always have as a first-time contributor to any project: Like not knowing adding an argument needed to be a test change or not noticing that there was a code formatter check I needed to please!