[OSE-177] Added GetOperation and ReviewSubmission impl #201
Conversation
andresuribe87
requested review from
decentralgabe and
nitro-neal
as code owners
pkg/server/router/operation.go
Outdated
|
|
||
| func serviceToRouterType(result interface{}) any { | ||
| switch result.(type) { | ||
| case model.Submission: |
There was a problem hiding this comment.
what's the point of this method?
There was a problem hiding this comment.
The idea is to once we have Operation object who's results are different types, this function will be responsible for the conversion. That said, I've removed it until that happens.
| switch { | ||
| case strings.Contains(op.ID, "submissions"): | ||
| var s prestorage.StoredSubmission | ||
| if err := json.Unmarshal(op.Response, &s); err != nil { |
There was a problem hiding this comment.
nit: consider returning an Operation pointer to avoid this
There was a problem hiding this comment.
I think sticking to non-pointers makes sense here. This we don't need to do nil-checking.
I reused the existing variable to avoid extra allocation.
There was a problem hiding this comment.
my stylistic preference but no dealbreaker 😄
| storage.UpdaterWithMap | ||
| } | ||
|
|
||
| func (u opUpdater) SetUpdatedResponse(first []byte) { |
| }), | ||
| }) | ||
| if err != nil { | ||
| return StoredSubmission{}, opstorage.StoredOperation{}, errors.Wrap(err, "updating value and operation") |
There was a problem hiding this comment.
nit: consider returning pointers to avoid needing to instantiate empty objects
There was a problem hiding this comment.
I always go back and forth with this. The nice thing about this is avoiding any possibility of dereferencing a nil pointer.
As you mentioned, the downside is instantiating empty objects. But I find that acceptable because it is typically very cheap, and most likely optimized away by the compiler.
There was a problem hiding this comment.
I think there's a semantic difference between an empty object and a nil object (e.g. empty set vs no set at all)
I agree it makes the caller API a bit more annoying because of the ptr dereferencing issue. I really wish you could get around this in go with an Optional type. There is a library but it's fairly un-go-like.
one of my favorite parts of scala was the option type which I believe has made its way into java and kotlin too
There was a problem hiding this comment.
Definitely agree that it's very much not idiomatic go to use option type. Having used them in the past, I view (*T, error) in go as exactly the same.
In Java world (I assume it's similar in Scala), there is a slight different between something like Optional<int> and Optional<Integer>, since in the latter, the object can be null. That's a similar problem as what we're discussing, I think.
| } | ||
|
|
||
| func (u UpdaterWithMap) Validate(v []byte) error { | ||
| return nil |
There was a problem hiding this comment.
hmm, is there any validation that can be done here?
There was a problem hiding this comment.
The only one I could think about was validating that v could be unmarshalled from json. But that seems like it's overkill. The design of Validate is so that it's a simple hook.
I considered an alternative approach, where you can pass in the validate function you want to run into the constructor of UpdaterWithMap. I like this approach better because it provides a default validating function.
There was a problem hiding this comment.
sounds good to me. may be worth adding a comment to capture that for future readers wondering if this can be removed
There was a problem hiding this comment.
Added comment.
pkg/storage/bolt.go
Outdated
| err := updateTxFn(namespace, key, updater, &first)(tx) | ||
| if err != nil { |
There was a problem hiding this comment.
nit: one liner here and below
Codecov Report
@@ Coverage Diff @@
## main #201 +/- ##
=========================================
+ Coverage 0 22.67% +22.67%
=========================================
Files 0 26 +26
Lines 0 2284 +2284
=========================================
+ Hits 0 518 +518
- Misses 0 1684 +1684
- Partials 0 82 +82
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Overview
This includes the implementation of the ReviewSubmission, and the GetOperation endpoints.
Description
Most of the meat and bones of this is in the
pkg/storage/bolt.gofile, where I made changes to operations and submissions are updated in a single transaction. Noteworthy items:pkg/service/presentation/model.gointo it's own package, to avoid import cycles.How Has This Been Tested?
See all test files modified. A key aspect is that once the operation is done, the
GetOperationwill return something like below:{ "id": "presentations/submissions/1237489", "done": true, "result": { "response": { "status":"approved", "reason":"fancy operator said it was ok", "id": ... "definition_id":... "definition_map":... } } }Checklist
Before submitting this PR, please make sure:
References
SIP6