[OSE-176] Operations can be cancelled #216

andresuribe87 · 2022-12-12T21:15:17Z

Overview

This PR implements the CancelOperation endpoint.

Description

After an operation is cancelled, the associated resource's status must be recorded. We currently only support operations related to Submission resources. In order to do so, this PR moved a number of methods around so there are no dependency cycles. The rule I followed was: submissions can depend on operations, but not the other way around.

Happy to hear feedback about better organizing packages.

How Has This Been Tested?

See added unit tests.

Checklist

Before submitting this PR, please make sure:

I have read the CONTRIBUTING document.
My code is consistent with the rest of the project
I have tagged the relevant reviewers and/or interested parties
I have updated the READMEs and other documentation of affected packages

References

SIP6

codecov-commenter · 2022-12-13T16:10:54Z

Codecov Report

Attention: Patch coverage is 12.76596% with 41 lines in your changes missing coverage. Please review.

Project coverage is 23.07%. Comparing base (f5ac154) to head (8353da1).
Report is 291 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/service/operation/submission/submission.go	17.64%	28 Missing ⚠️
pkg/server/router/operation.go	0.00%	13 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #216      +/-   ##
==========================================
+ Coverage   22.67%   23.07%   +0.39%     
==========================================
  Files          26       25       -1     
  Lines        2293     2254      -39     
==========================================
  Hits          520      520              
+ Misses       1691     1652      -39     
  Partials       82       82

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nitro-neal · 2022-12-13T16:56:42Z

pkg/service/operation/submission/submission.go

+const ParentResource = "/presentations/submissions"
+
+// Status indicates the current state of a submission.
+type Status uint8


why a uint8 and not define a type for status

type (
Status string
)

const (
StatusPending Status = "pending"
StatusDenied Status = "denied"
)

Synced offline. We're storing uints, and exposing strings.

decentralgabe · 2022-12-13T17:36:28Z

pkg/service/operation/bolt.go

+	db *storage.BoltDB
+}
+
+func (b BoltOperationStorage) CancelOperation(id string) (opstorage.StoredOperation, error) {


Suggested change

func (b BoltOperationStorage) CancelOperation(id string) (opstorage.StoredOperation, error) {

func (b BoltOperationStorage) CancelOperation(id string) (*opstorage.StoredOperation, error) {

decentralgabe · 2022-12-13T19:14:59Z

pkg/service/operation/bolt.go

+		_, opData, err := b.db.UpdateValueAndOperation(
+			submission.Namespace, submission.ResourceID(id), storage.NewUpdater(map[string]any{
+				"status": submission.StatusCancelled,
+				"reason": cancelledReason,


when is this reason ever used/surfaced?

GetSubmission returns an object that has a reason field. It's meant to be a simple record and can be empty.

think it'll be useful for auditing purposes

decentralgabe · 2022-12-13T19:15:26Z

pkg/service/operation/bolt.go

+	if err != nil {
+		return util.LoggingErrorMsgf(err, "marshalling operation with id: %s", id)
+	}
+	return b.db.Write(namespace.FromID(id), id, jsonBytes)


is it worth pulling this off as a separate value to be able to wrap the error with a log in case it fails?

This file was actually moved only. That said, I've updated this to wrap and log.

decentralgabe · 2022-12-13T19:16:15Z

pkg/service/operation/bolt.go

+		if err != nil {
+			stored = append(stored, nextOp)
+			continue
+		}
+		if include {
+			stored = append(stored, nextOp)
+		}


Suggested change

if err != nil {

stored = append(stored, nextOp)

continue

}

if include {

stored = append(stored, nextOp)

}

if err != nil || include {

stored = append(stored, nextOp)

}

decentralgabe · 2022-12-13T19:16:49Z

pkg/service/operation/bolt.go

+	return nil
+}
+
+func NewBoltOperationStorage(db *storage.BoltDB) (*BoltOperationStorage, error) {


nit: I usually put the constructors at the top of the file

decentralgabe · 2022-12-13T19:17:08Z

pkg/service/operation/service.go

@@ -102,8 +103,20 @@ func (s Service) GetOperation(request GetOperationRequest) (Operation, error) {
 	return serviceModel(storedOp)
 }

+func (s Service) CancelOperation(request CancelOperationRequest) (Operation, error) {


Suggested change

func (s Service) CancelOperation(request CancelOperationRequest) (Operation, error) {

func (s Service) CancelOperation(request CancelOperationRequest) (*Operation, error) {

decentralgabe · 2022-12-13T19:17:48Z

pkg/service/operation/submission/submission.go

+const Namespace = "presentation_submission"
+
+// ParentResource is the prefix of the submission parent resource.
+const ParentResource = "/presentations/submissions"


nit could put these in the same const () block

pkg/service/operation/submission/submission.go

decentralgabe · 2022-12-14T22:50:58Z

pkg/server/server_presentation_test.go

 						DefinitionID: definition.PresentationDefinition.ID,
 					},
 				},
 				{
 					Status: "pending",
 					PresentationSubmission: &exchange.PresentationSubmission{
-						ID:           operation.SubmissionID(op2.ID),
+						ID:           submission.ResourceID(op2.ID),


why resource over submission? submission is more meaningful in context

Renamed to ID, as submission is already in the package name.

decentralgabe · 2022-12-14T22:52:41Z

pkg/service/operation/service.go

@@ -102,8 +103,20 @@ func (s Service) GetOperation(request GetOperationRequest) (Operation, error) {
 	return serviceModel(storedOp)
 }

+func (s Service) CancelOperation(request CancelOperationRequest) (Operation, error) {
+	if err := request.Validate(); err != nil {
+		return Operation{}, errors.Wrap(err, "invalid request")


Suggested change

return Operation{}, errors.Wrap(err, "invalid request")

return nil, errors.Wrap(err, "invalid request")

decentralgabe · 2022-12-14T22:53:08Z

pkg/service/operation/service.go

+
+	storedOp, err := s.storage.CancelOperation(request.ID)
+	if err != nil {
+		return Operation{}, errors.Wrap(err, "marking as done")


Suggested change

return Operation{}, errors.Wrap(err, "marking as done")

return nil, errors.Wrap(err, "marking as done")

decentralgabe

approving pending few small comments

* modity storage * modify tests * modify service storage * change module name * update * fix warnings * [OSE-176] Operations can be cancelled (#216) * Initial draft of cancellation * Linters * Moving a constant. * Better homes for many methods and variables. * PR comments * Lets make pointers. * Rename func * add schema caching (#219) Co-authored-by: Andres Uribe <[email protected]> * modity storage * modify tests * modify service storage * change module name * update * fix warnings * fix server test * Fixes to the merge problems. * Small fixes * Lint Co-authored-by: Andres Uribe <[email protected]> Co-authored-by: Gabe <[email protected]> Co-authored-by: Andres Uribe Gonzalez <[email protected]>

andresuribe87 added 4 commits December 12, 2022 15:57

Initial draft of cancellation

12310d8

Linters

5fc7ebf

Moving a constant.

dbd960e

Better homes for many methods and variables.

03e6363

andresuribe87 marked this pull request as ready for review December 13, 2022 16:09

andresuribe87 requested review from decentralgabe and nitro-neal as code owners December 13, 2022 16:09

nitro-neal reviewed Dec 13, 2022

View reviewed changes

nitro-neal approved these changes Dec 13, 2022

View reviewed changes

decentralgabe reviewed Dec 13, 2022

View reviewed changes

pkg/service/operation/submission/submission.go Show resolved Hide resolved

andresuribe87 and others added 2 commits December 14, 2022 16:10

PR comments

7a9e4f4

Merge branch 'main' into ose-176

76b06e7

decentralgabe reviewed Dec 14, 2022

View reviewed changes

decentralgabe approved these changes Dec 14, 2022

View reviewed changes

andresuribe87 added 2 commits December 15, 2022 05:16

Lets make pointers.

4369608

Rename func

8353da1

andresuribe87 merged commit 384f92e into TBD54566975:main Dec 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OSE-176] Operations can be cancelled #216

[OSE-176] Operations can be cancelled #216

andresuribe87 commented Dec 12, 2022 •

edited

Loading

codecov-commenter commented Dec 13, 2022 •

edited

Loading

nitro-neal Dec 13, 2022 •

edited

Loading

andresuribe87 Dec 14, 2022

decentralgabe Dec 13, 2022

andresuribe87 Dec 14, 2022

decentralgabe Dec 13, 2022

andresuribe87 Dec 14, 2022

decentralgabe Dec 14, 2022

decentralgabe Dec 13, 2022

andresuribe87 Dec 14, 2022

decentralgabe Dec 13, 2022

andresuribe87 Dec 14, 2022

decentralgabe Dec 13, 2022

andresuribe87 Dec 14, 2022

decentralgabe Dec 13, 2022

andresuribe87 Dec 14, 2022

decentralgabe Dec 13, 2022

andresuribe87 Dec 14, 2022

decentralgabe Dec 14, 2022

andresuribe87 Dec 15, 2022

decentralgabe Dec 14, 2022

andresuribe87 Dec 15, 2022

decentralgabe Dec 14, 2022

andresuribe87 Dec 15, 2022

decentralgabe left a comment

	func (b BoltOperationStorage) CancelOperation(id string) (opstorage.StoredOperation, error) {
	func (b BoltOperationStorage) CancelOperation(id string) (*opstorage.StoredOperation, error) {

	func (s Service) CancelOperation(request CancelOperationRequest) (Operation, error) {
	func (s Service) CancelOperation(request CancelOperationRequest) (*Operation, error) {

	return Operation{}, errors.Wrap(err, "invalid request")
	return nil, errors.Wrap(err, "invalid request")

	return Operation{}, errors.Wrap(err, "marking as done")
	return nil, errors.Wrap(err, "marking as done")

[OSE-176] Operations can be cancelled #216

[OSE-176] Operations can be cancelled #216

Conversation

andresuribe87 commented Dec 12, 2022 • edited Loading

Overview

Description

How Has This Been Tested?

Checklist

References

codecov-commenter commented Dec 13, 2022 • edited Loading

Codecov Report

nitro-neal Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decentralgabe left a comment

Choose a reason for hiding this comment

andresuribe87 commented Dec 12, 2022 •

edited

Loading

codecov-commenter commented Dec 13, 2022 •

edited

Loading

nitro-neal Dec 13, 2022 •

edited

Loading