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

server: return file on ACH file creation #1063

Closed
wants to merge 1 commit into from

Conversation

zbruhnke
Copy link

@zbruhnke zbruhnke commented Aug 2, 2022

We were talking about how it would be nice to get the JSON representation of the file back on file creation in slack the other day, since no data is persisted this seems like it would be a worthwhile add.

If accepted I'd like do the same to the segment endpoints so it returns both files

@zbruhnke zbruhnke requested a review from vxio as a code owner August 2, 2022 23:09
@@ -100,9 +101,11 @@ func createFileEndpoint(s Service, r Repository, logger log.Logger) endpoint.End
}
}

f, _ := r.FindFile(req.File.ID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we avoided this because the file often won't be valid. The code isn't calling Create()/build() so the returned result isn't useful. Also if the caller submits a valid file, then they have that file already. Converting To/From as a wasteful step didn't seem useful to me.

Copy link
Author

@zbruhnke zbruhnke Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, let me be more explicit here - the reason for this is because we might have the valid file BUT we also need that file to exist in your system in order to segment it

so let me use an RDFI workflow use case as an example

  1. File comes in from Fed
  2. File is streamed to us from achgateway in valid Moov JSON Nacha formatting
  3. we now take that value and we have to create the file so that it exists in your system in order for us to segment it

If this is not the right use-case then the best thing we could do is depend on ACH gateway to ALWAYS give us a valid file and allow me to upload a file in the body to the segment endpoint.

In that case though, it would still also be useful to still return the entire valid file for the debits and the credits.

If you'd accept a PR for that workflow (accepting a file in the body and returning the valid file json) then I would consider working on that instead

Let me know if that makes sense

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That workflow makes a lot of sense and not having interop between achgateway and ach's endpoints causes these additional calls.

Are you asking for an endpoint to submit and file and return the segmented files back? Bypassing the need to Create and then Segment?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamdecaf yes, let me share some code so you could understand a bit more the full context

def handle(%RdfiAchNachaFileReceived{} = event, metadata) do
    with {:ok, file, _} <- @moov_ach_client.create_file(event.nacha),
         {:ok, segmented_file, _} <- @moov_ach_client.segment_file(file.id),
         {:ok, credit_file, _} <- @moov_ach_client.get_file(segmented_file.creditFileID),
         {:ok, debit_file, _} <- @moov_ach_client.get_file(segmented_file.debitFileID),
         do: dispatch_segment_rdfi_ach_nacha_file(credit_file, debit_file, event, metadata)
  end

Notice all the Moov ACH calls to get the segmented files out. Ideally, it could be reduced to one call that gives me the segmentation of it taking the event.nacha payload.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with this change you'd need to make multiple calls in order to segment a file. This issue was not about segmenting the file without first creating it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your issue is more about trying to translate the entire moov-io/ach library to other languages. The HTTP server was designed for basic usage -- not as a complete implementation. That's why it doesn't have storage or a full feature set.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamdecaf We're not at all trying to translate this to other languages.

Here's what what we're doing (notice the client calls)

  1. Submitting a valid ACH file (given to us from ACH gateway in the Kafka stream) to ACH via POST to /files/create
  2. Segmenting that created file via a POST to /files/{fileId}/segment
  3. Retrieving the credit file if returned via a GET to /files/{fileId}/contents
  4. Retrieving the debit file if returned via a GET to /files/{fileId}/contents

What Ubi is saying is that in an ideal world we would instead submit a valid file which we received from ACH Gateway's Kafka stream stright to the segment endpoint in Moov ACH and then return both whole files for debit and credit like the below

  1. Submitting a valid ACH file (given to us from ACH gateway in the Kafka stream) to ACH via POST to /file/{fileId}/segment or some equivalent endpoint that will accept a body that is a valid ACH File in Moov's JSON format
  2. That endpoint would return a struct that contained a creditFile and a debitFile in their entirety so we could then split them up into individual transactions and post them to our core system

It not having storage is exactly the reason we were saying it might be better to return the files on these calls, so that they could not get "lost" between creating and retrieving them

Does that make more sense?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense, but we can't break the existing API's so the segment endpoint would need to be a new one.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok that seems fair - thanks for the feedback, I'll put that in as a separate MR when I get some time!

@adamdecaf adamdecaf changed the title return file on ACH file creation server: return file on ACH file creation Aug 9, 2022
@adamdecaf
Copy link
Member

I've created #1067 which implements this for a few endpoints.

@adamdecaf adamdecaf added this to the v1.19 milestone Aug 17, 2022
@adamdecaf adamdecaf closed this Aug 17, 2022
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.

3 participants