personal-finance/datastore
2
0
Fork 0
Code Issues 12 Pull Requests Actions Packages Releases Wiki Activity

Implements /transactions PUT method #22

Merged
satprog merged 1 commits from feature/20-transactions-put into main 2024-08-03 22:21:19 +00:00
Conversation 6 Commits 1 Files Changed 8 +358 -18
satprog commented 2024-06-22 20:52:17 +00:00
Owner
Copy Link

Implements /transactions PUT method

Adds PUT method to OpenAPI spec.

Given that the transaction IDs are generated on server-side, for the PUT
method to remain idempotent, it can only update existing transactions.

It also adds a TransactionExists method on the DAL.

Issue: #20

Implements /transactions PUT method Adds PUT method to OpenAPI spec. Given that the transaction IDs are generated on server-side, for the PUT method to remain idempotent, it can only update existing transactions. It also adds a TransactionExists method on the DAL. Issue: #20
satprog added this to the /transactions milestone 2024-06-22 20:52:17 +00:00
satprog added the
enhancement
 label 2024-06-22 20:52:17 +00:00
satprog added 2 commits 2024-06-22 20:52:18 +00:00 
It adds the method to the OpenAPI spec and generates a new server
config. The requirement for the ID on the Transaction component is
removed, so that it can be reused for insertions.
It also adds two new middlewares, a logging and a spec validator. If a
request does not follow the spec, a 400 is returned immediately.

Issue: #18
Adds PUT method to OpenAPI spec.

Given that the transaction IDs are generated on server-side, for the PUT
method to remain idempotent, it can only update existing transactions.

It also adds a TransactionExists method on the DAL.

Issue: #20
satprog removed the
enhancement
 label 2024-06-22 20:52:29 +00:00
satprog changed title from WIP: feature/20-transactions-put to feature/20-transactions-put 2024-06-22 20:52:56 +00:00
satprog changed title from feature/20-transactions-put to Implements /transactions PUT method 2024-06-22 20:53:22 +00:00
satprog force-pushed feature/20-transactions-put from 35b35f5e40 to 55bd87c864 2024-06-22 21:30:08 +00:00 Compare
satprog added this to the Client-Server architecture project 2024-06-22 21:31:46 +00:00
satprog requested review from nunorosa 2024-06-22 21:31:59 +00:00
satprog added a new dependency 2024-06-22 21:32:25 +00:00
#20 Implement PUT transactions
satprog removed a dependency 2024-06-22 21:32:47 +00:00
#20 Implement PUT transactions
satprog added a new dependency 2024-06-22 21:32:53 +00:00
#19 Implements /transactions POST method
satprog added a new dependency 2024-06-22 21:38:22 +00:00
#25 Fix resources naming scheme
satprog removed this from the Client-Server architecture project 2024-06-22 21:45:39 +00:00
satprog removed this from the /transactions milestone 2024-06-23 19:52:25 +00:00
nunorosa reviewed 2024-06-25 21:13:45 +00:00
docs/openapi.yaml
@ -59,0 +85,4 @@
- required:
- id
responses:
nunorosa commented 2024-06-25 21:13:45 +00:00
Collaborator
Copy Link

Is this the only response allowed? If you pass an Id that does not exist what would be the expected response?

Is this the only response allowed? If you pass an Id that does not exist what would be the expected response?
satprog commented 2024-06-29 10:34:55 +00:00
Author
Owner
Copy Link

Fixed on #25. I was initially thinking that a PUT could also create transactions, however the recommendation from the spec is that when the URI of a resource is generated server-side, creations should be implemented in the POST method only. 4.3.4. PUT

Fixed on #25. I was initially thinking that a PUT could also create transactions, however the recommendation from the spec is that when the URI of a resource is generated server-side, creations should be implemented in the POST method only. [4.3.4. PUT](https://www.rfc-editor.org/rfc/rfc7231#section-4.3.4)
nunorosa marked this conversation as resolved
nunorosa reviewed 2024-06-25 21:21:58 +00:00
internal/api/converter.go
@ -11,0 +27,4 @@
}
return entity.Transaction{
Id: id,
nunorosa commented 2024-06-25 21:21:58 +00:00
Collaborator
Copy Link

Does it make sense to create a entity.Transaction with an invalid Id. If the Transaction provided as argument has an t.Id == nil then the entity.Transaction we create is nil. Could this lead to more trouble down the line? Would it make sense to "throw" or have some kind of error message here?

Does it make sense to create a entity.Transaction with an invalid Id. If the Transaction provided as argument has an t.Id == nil then the entity.Transaction we create is nil. Could this lead to more trouble down the line? Would it make sense to "throw" or have some kind of error message here?
satprog commented 2024-06-29 11:06:26 +00:00
Author
Owner
Copy Link

You raise a good question. Logically, I don't think it's possible to have an invalid ID, it should be prevented by the OpenAPI spec. I think at the time this stayed like this to facilitate the type going into the DAL. This would only be an invalid ID when the actual ID is getting created at the DB.
But I could imagine this being an issue down the line, you can create a techdebt and/or question issue on the repo.

PS: this was on the previous commit, can't change it now.

You raise a good question. Logically, I don't think it's possible to have an invalid ID, it should be prevented by the OpenAPI spec. I think at the time this stayed like this to facilitate the type going into the DAL. This would only be an invalid ID when the actual ID is getting created at the DB. But I could imagine this being an issue down the line, you can create a techdebt and/or question issue on the repo. PS: this was on the previous commit, can't change it now.
nunorosa marked this conversation as resolved
nunorosa reviewed 2024-06-25 21:27:22 +00:00
internal/api/impl.go
@ -77,0 +76,4 @@
return ctx.JSON(http.StatusOK, entity2transaction(*transaction))
}
func (server *ServerImpl) CreateTransaction(ctx echo.Context) error {
nunorosa commented 2024-06-25 21:27:22 +00:00
Collaborator
Copy Link

Does "error" correspond to the output of this function? Is this name by default? I feel like since you may get error or sucess status code the name could be StatusCode or JSONResult or something like that. But I'm not sure. :)

Does "error" correspond to the output of this function? Is this name by default? I feel like since you may get error or sucess status code the name could be StatusCode or JSONResult or something like that. But I'm not sure. :)
satprog commented 2024-06-29 11:09:28 +00:00
Author
Owner
Copy Link

This is idiomatic Go. If the error == nil, then the function worked. https://go.dev/doc/effective_go#errors

This is idiomatic Go. If the error == nil, then the function worked. https://go.dev/doc/effective_go#errors
nunorosa marked this conversation as resolved
nunorosa reviewed 2024-06-25 21:33:45 +00:00
internal/api/impl.go
@ -77,0 +107,4 @@
return ctx.NoContent(http.StatusInternalServerError)
}
if exists {
nunorosa commented 2024-06-25 21:33:45 +00:00
Collaborator
Copy Link

I would have inverted this if condition. It would have lowered the number of cascaded ifs below. Not that this is a problem. The number of nested ifs is really small :)

I would have inverted this if condition. It would have lowered the number of cascaded ifs below. Not that this is a problem. The number of nested ifs is really small :)
satprog commented 2024-06-29 10:35:49 +00:00
Author
Owner
Copy Link

Removed on #25.

Removed on #25.
nunorosa marked this conversation as resolved
nunorosa reviewed 2024-06-25 21:40:52 +00:00
nunorosa left a comment
Collaborator
Copy Link

I left some small comments which do not affect in the least the fantastic quality of this commit! 5 stars. Would recommend on trip advisor. 😄

I left some small comments which do not affect in the least the fantastic quality of this commit! 5 stars. Would recommend on trip advisor. 😄
😆 1
nunorosa approved these changes 2024-07-26 10:27:32 +00:00
satprog merged commit ac396ac259 into main 2024-08-03 22:21:19 +00:00
satprog deleted branch feature/20-transactions-put 2024-08-03 22:21:19 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
nunorosa
Labels
Clear labels
bug
Something is not working
duplicate
This issue or pull request already exists
enhancement
New feature
help wanted
Need some help
invalid
Something is wrong
question
More information is needed
techdebt
Technical debt
wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
joanaosoriosousa nunorosa satprog
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Blocks
#25 Fix resources naming scheme
personal-finance/datastore
Depends on
#19 Implements /transactions POST method
personal-finance/datastore
Reference: personal-finance/datastore#22
No description provided.
Delete Branch "feature/20-transactions-put"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?