search

moov-io / achgateway

Payment gateway enabling automated ACH operations in a distributed and fault tolerant way.
https://moov-io.github.io/achgateway/
Apache License 2.0
54 stars 19 forks source link

Support nested inbound directory structures #158

Closed jrnt30 closed 1 year ago

jrnt30 commented 1 year ago

Changes

Adds in the ability to have a nested directory as the source of incoming files.

Why Are Changes Being Made

Currently when processing an inbound folder that is a nested directory (ex: "inbound": "/client_directory/with/subpath", the processing of the files fails due to processFile being called with a directory instead of a specific file. This is not a fair assumption if the InboundPath(), ReconciliationPath(), etc. have any "depth" to them.

In other places we make use explicitly of the agent's path information to determine what folders/files to operate on. Following a similar pattern here.

NOTE: The way that the audit folder path is constructed in processFile currently does not show the full/nested path but rather just the "last" directory in this hierarchy. If the full path would be preferred (debatable) can also work on making some of those changes.

jrnt30 commented 1 year ago

Noticed that there are some warnings now being thrown for recursive delete for the base directories as well, will take a look and get it fully working.

yordis commented 1 year ago

Please make this a required to opt-in feature since it will break changes for some people like us 🙏🏻

jrnt30 commented 1 year ago

@yordis Appreciate the feedback and nudge for a configuration parameter. I updated the description of the issue to ensure it's clear the current and adjusted functionality present, it was a bit surprising to us the way it was currently working.

I don't believe that the changes proposed here should break an existing setup unless you have a folder structure on the server that contains nested directories in which case I think you would already be seeing the same issue that we are. Please let me know though if I am missing something!

(Also as a side note, if it is preferable to be more explicit it ProcessFiles to operate directly off of the paths of the agent we can go that route too.)

jrnt30 commented 1 year ago

I decided to just go ahead and adjust to using the explicit folder structure. Let me know if this feels safer/better.

codecov-commenter commented 1 year ago

Codecov Report

Base: 40.81% // Head: 40.81% // Increases project coverage by +0.00% :tada:

Coverage data is based on head (319a37d) compared to base (b8e8663). Patch coverage: 62.50% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #158 +/- ## ======================================= Coverage 40.81% 40.81% ======================================= Files 90 90 Lines 3891 3876 -15 ======================================= - Hits 1588 1582 -6 + Misses 2008 2001 -7 + Partials 295 293 -2 ``` | [Impacted Files](https://codecov.io/gh/moov-io/achgateway/pull/158?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io) | Coverage Δ | | |---|---|---| | [internal/incoming/odfi/scheduler.go](https://codecov.io/gh/moov-io/achgateway/pull/158/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-aW50ZXJuYWwvaW5jb21pbmcvb2RmaS9zY2hlZHVsZXIuZ28=) | `31.91% <0.00%> (ø)` | | | [internal/incoming/odfi/processor.go](https://codecov.io/gh/moov-io/achgateway/pull/158/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-aW50ZXJuYWwvaW5jb21pbmcvb2RmaS9wcm9jZXNzb3IuZ28=) | `73.33% <71.42%> (+7.77%)` | :arrow_up: | | [internal/upload/sftp.go](https://codecov.io/gh/moov-io/achgateway/pull/158/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io#diff-aW50ZXJuYWwvdXBsb2FkL3NmdHAuZ28=) | `65.47% <0.00%> (-0.90%)` | :arrow_down: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=moov-io)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

adamdecaf commented 1 year ago

Has this been working for you?

yordis commented 1 year ago

@jrnt30 based on my career experience, the fewer assumptions, the better. These configurations are set up once every few times or once and never change.

In case of the changes, you probably want things to break if it wasn't expected.

So, your solution is something I would do. We use Terraform and Helm to provision things, so there is a minimal cost to it. We do not have any dynamic directory creation ever.

jrnt30 commented 1 year ago

It has been working for us, but wanted to call out two things.

The FTP client cds into the different paths and pulls the files from that directory locally. This functions properly with our setup of having a non-root working directory for the FTP user's default path as well as a relative path to the folders we want to process.

The SFTP agent's code lists the files for each of the directory using the user's working directory joined with the relative paths. It then uses the relative path to attempt to Stat the file and download it, however when doing so the path that was constructed from the file listing does not pass the .Stat command.

adamdecaf commented 1 year ago

Has this been working for folks? My bad it hasn't been merged. I'm reading the comments again to understand what the tradeoffs are.

Edit: Can we rebase this PR off of the master branch? There have been some quality of life improvements and fixes.

adamdecaf commented 1 year ago

The SFTP agent's code lists the files for each of the directory using the user's working directory joined with the relative paths. It then uses the relative path to attempt to Stat the file and download it, however when doing so the path that was constructed from the file listing does not pass the .Stat command.

Sounds like we need to resolve the path within the SFTP server before trying to stat?

adamdecaf commented 1 year ago

Thanks for updating this PR. I'm comfortable merging if you think it's ready.

jrnt30 commented 1 year ago

Hey Adam! Sorry for the delayed response, have been OOO for a bit. This has been working for us with the caveat that we adjusted to use absolute paths for the directories.

An example that does not work:

Server with a base root FTP folder of: / User with a default working directly different: /user/jrnt30/ Any xPath that is relative and not absolute: ./files

If the project would like to support both relative paths to the user's working directory, there is additional work that would need to be done to get SFTP working to match the behavior of the FTP processor. I looked at it briefly but didn't see any similar cd like capability in the Golang SFTP client and stopped pursuing it.

adamdecaf commented 1 year ago

Right. The SFTP client doesn't support cd like behavior. I'm thinking that we want both clients to support:

adamdecaf commented 1 year ago

In your example I'd expect the resolved path to be /user/jrnt30/files, right?

jrnt30 commented 1 year ago

In your example I'd expect the resolved path to be /user/jrnt30/files, right?

Yep!

Right. The SFTP client doesn't support cd like behavior. I'm thinking that we want both clients to support:

  • Absolute path provided. (e.g. /data/returned/)
  • Relative path from user's home (e.g. ./returned/)

Is support for both path types something you would like to see in the PR prior to merging? One of the things I have not explored is what visibility we have into the CWD of a user's default home directory to construct a fully qualified path we might need.

IIRC there were some differences in how SFTP and FTP worked for these different relative vs. absolute path types as well. I had started trying to replicate our SFTP setup a bit more directly in the Docker Compose setup included but haven't looked at it in quite a while. If this is something you'd like to tackle now I can try and get a few hours over the next week or two to contribute an example.

jvantuyl commented 1 year ago

FYI, I just got bit by this and our banking partner isn't going to change their directory structure to be unnested. Any idea if this is going any time soon?

adamdecaf commented 1 year ago

Yea, I can release this today. It seems safe enough to deploy.