[OSE-166] Scaffolding for /v1/presentations/submissions #171

andresuribe87 · 2022-11-11T14:16:56Z

This adds the routing for Create, Get, List, and Review endpoints for submissions. None of the business logic nor tests were added. Only simple routing, request types, and response types.

codecov-commenter · 2022-11-11T14:19:34Z

Codecov Report

Attention: Patch coverage is 34.61538% with 17 lines in your changes missing coverage. Please review.

Project coverage is 26.52%. Comparing base (3c91cb6) to head (4ef3ccf).
Report is 319 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/server/router/presentation.go	5.55%	17 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
+ Coverage   26.37%   26.52%   +0.15%     
==========================================
  Files          21       21              
  Lines        1623     1640      +17     
==========================================
+ Hits          428      435       +7     
- Misses       1123     1133      +10     
  Partials       72       72

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

decentralgabe · 2022-11-14T18:17:54Z

pkg/server/router/submission.go

+}
+
+type CreateSubmissionRequest struct {
+	PresentationJwt keyaccess.JWT `json:"presentationJwt" validate:"required"`


nit: PresentationJWT

question: this better as SubmissionJWT?

Used SubmissionJWT

decentralgabe · 2022-11-14T18:22:47Z

pkg/server/router/submission.go

+// @Failure      500      {string}  string  "Internal server error"
+// @Router       /v1/presentations/submissions [put]
+func (sr SubmissionRouter) CreateSubmission(ctx context.Context, w http.ResponseWriter, r *http.Request) error {
+	resp := Operation{}


var resp Operation

https://github.com/uber-go/guide/blob/master/style.md#use-var-for-zero-value-structs

note - don't think our README points to this style doc even though it should. may need to add it

Done. Heavy +1 to adding it as well.

ty - feel free to add to the README

decentralgabe · 2022-11-14T18:23:11Z

pkg/server/router/submission.go

+// @Failure      400  {string}  string  "Bad request"
+// @Router       /v1/presentations/submission/{id} [get]
+func (sr SubmissionRouter) GetSubmission(ctx context.Context, w http.ResponseWriter, r *http.Request) error {
+	resp := GetSubmissionResponse{}


Suggested change

resp := GetSubmissionResponse{}

var resp GetSubmissionResponse

decentralgabe · 2022-11-14T18:23:49Z

pkg/server/router/submission.go

+// @Failure      500  {string}  string  "Internal server error"
+// @Router       /v1/presentations/submissions [get]
+func (sr SubmissionRouter) ReviewSubmission(ctx context.Context, w http.ResponseWriter, r *http.Request) error {
+	return framework.Respond(ctx, w, ReviewSubmissionResponse{}, http.StatusOK)


is there value in putting ReviewSubmissionResponse{} over nil?

I don't think so. That said, this will change when the implementation is done.

decentralgabe · 2022-11-14T18:24:52Z

pkg/server/server.go

 		return s.PresentationAPI(service)
+	case svcframework.Submission:
+		return s.SubmissionAPI(service)


I had a working assumption that the PresentationAPI was going to be including the SubmissionAPI similar to how the ApplicationAPI is under the ManifestAPI. They should be the same service because they are a part of the same spec/group of functionality IMO

Done. Grouped everything under the same API.

decentralgabe · 2022-11-14T18:25:09Z

pkg/server/server.go

+
+	return
+}
+func (s *SSIServer) SubmissionAPI(service svcframework.Service) (err error) {


along with above - would like to see this a part of the PresentationAPI

decentralgabe · 2022-11-14T18:25:45Z

pkg/service/submission/service.go

+	submissionStorage, err := submissionstorage.NewSubmissionStorage(s)
+	if err != nil {
+		errMsg := "could not instantiate storage for the submission service"
+		return nil, util.LoggingErrorMsg(err, errMsg)


could include the errMsg inline here

decentralgabe · 2022-11-14T18:25:53Z

pkg/service/submission/service.go

+
+	if !request.IsValid() {
+		errMsg := fmt.Sprintf("invalid create presentation submission request: %+v", request)
+		return nil, util.LoggingNewError(errMsg)


could use LoggingNewErrorf

decentralgabe · 2022-11-14T18:26:10Z

pkg/service/submission/service.go

+	storedSubmission, err := s.storage.GetSubmission(request.ID)
+	if err != nil {
+		err := errors.Wrapf(err, "error getting presentation submission: %s", request.ID)
+		return nil, util.LoggingError(err)


could use LoggingNewErrorf

decentralgabe · 2022-11-14T18:26:24Z

pkg/service/submission/service.go

+	}
+	if storedSubmission == nil {
+		err := fmt.Errorf("presentation submission with id<%s> could not be found", request.ID)
+		return nil, util.LoggingError(err)


could use LoggingErrorf

Used LoggingNewErrorf

decentralgabe · 2022-11-14T18:26:36Z

pkg/service/submission/storage/bolt.go

+	namespace = "presentation_submission"
+)
+
+type BoltSubmissionStorage struct {


think this should be merged into the presentation storage

decentralgabe

see comments

andresuribe87

@decentralgabe PTAL

andresuribe87 · 2022-11-14T18:54:57Z

pkg/server/router/submission.go

+// @Failure      500      {string}  string  "Internal server error"
+// @Router       /v1/presentations/submissions [put]
+func (sr SubmissionRouter) CreateSubmission(ctx context.Context, w http.ResponseWriter, r *http.Request) error {
+	resp := Operation{}


Done. Heavy +1 to adding it as well.

andresuribe87 · 2022-11-14T18:56:42Z

pkg/server/router/submission.go

+// @Failure      400  {string}  string  "Bad request"
+// @Router       /v1/presentations/submission/{id} [get]
+func (sr SubmissionRouter) GetSubmission(ctx context.Context, w http.ResponseWriter, r *http.Request) error {
+	resp := GetSubmissionResponse{}


andresuribe87 · 2022-11-14T18:58:21Z

pkg/server/router/submission.go

+// @Failure      500  {string}  string  "Internal server error"
+// @Router       /v1/presentations/submissions [get]
+func (sr SubmissionRouter) ReviewSubmission(ctx context.Context, w http.ResponseWriter, r *http.Request) error {
+	return framework.Respond(ctx, w, ReviewSubmissionResponse{}, http.StatusOK)


I don't think so. That said, this will change when the implementation is done.

andresuribe87 · 2022-11-14T19:03:23Z

pkg/server/server.go

 		return s.PresentationAPI(service)
+	case svcframework.Submission:
+		return s.SubmissionAPI(service)


Done. Grouped everything under the same API.

andresuribe87 · 2022-11-14T20:08:32Z

pkg/server/server.go

+
+	return
+}
+func (s *SSIServer) SubmissionAPI(service svcframework.Service) (err error) {


andresuribe87 · 2022-11-14T20:10:51Z

pkg/service/submission/service.go

+
+	if !request.IsValid() {
+		errMsg := fmt.Sprintf("invalid create presentation submission request: %+v", request)
+		return nil, util.LoggingNewError(errMsg)


andresuribe87 · 2022-11-14T20:14:23Z

pkg/service/submission/service.go

+	storedSubmission, err := s.storage.GetSubmission(request.ID)
+	if err != nil {
+		err := errors.Wrapf(err, "error getting presentation submission: %s", request.ID)
+		return nil, util.LoggingError(err)


andresuribe87 · 2022-11-14T20:14:45Z

pkg/service/submission/service.go

+	}
+	if storedSubmission == nil {
+		err := fmt.Errorf("presentation submission with id<%s> could not be found", request.ID)
+		return nil, util.LoggingError(err)


Used LoggingNewErrorf

andresuribe87 · 2022-11-14T20:14:52Z

pkg/service/submission/storage/bolt.go

+	namespace = "presentation_submission"
+)
+
+type BoltSubmissionStorage struct {


andresuribe87 · 2022-11-14T20:16:49Z

pkg/server/router/submission.go

+}
+
+type CreateSubmissionRequest struct {
+	PresentationJwt keyaccess.JWT `json:"presentationJwt" validate:"required"`


Used SubmissionJWT

* The simplest scaffolding possible * rename Co-authored-by: Gabe <gcohen@squareup.com>

decentralgabe · 2022-11-14T22:59:22Z

pkg/server/router/presentation.go

+}
+
+type ListSubmissionRequest struct {
+	Filter string `json:"filter"`


where's the right place to put doc on how the filter syntax works?

Added below.

decentralgabe · 2022-11-14T23:00:00Z

pkg/server/router/presentation.go

+
+// ListSubmissions godoc
+// @Summary      List Submissions
+// @Description  List existing submissions according to a filtering query.


maybe here with a link to the filter syntax we use

Added here. Will likely expand on this once I do the implementation.

decentralgabe · 2022-11-14T23:00:37Z

pkg/service/presentation/service.go

 	presentationStorage, err := presentationstorage.NewPresentationStorage(s)
 	if err != nil {
-		errMsg := "could not instantiate storage for the presentation definition service"
-		return nil, util.LoggingErrorMsg(err, errMsg)
+		return nil, util.LoggingErrorMsg(err, "could not instantiate storage for the presentation definition service")


presentation service

decentralgabe · 2022-11-14T23:03:53Z

pkg/service/presentation/storage/bolt.go

+	if err != nil {
+		errMsg := fmt.Sprintf("could not store submission definition: %s", id)
+		logrus.WithError(err).Error(errMsg)
+		return errors.Wrapf(err, errMsg)


nit: could use the util.Logging... whatever as it combines the error creation, logging, and error return here and the rest of this file. but NBD

decentralgabe

small comments, approving

andresuribe87 added 3 commits November 11, 2022 09:13

Initial create, delete, get for submissions

8b7a1d0

Basic tests for the router

18c6eab

Added the rest of the scaffolding

4820940

andresuribe87 added 3 commits November 11, 2022 09:32

Added List and Review

1988345

Removed the delete method

b485e80

Simplified for scaffolding

9ed5231

andresuribe87 marked this pull request as ready for review November 11, 2022 15:16

andresuribe87 requested review from decentralgabe and nitro-neal as code owners November 11, 2022 15:16

Gabe and others added 3 commits November 11, 2022 11:26

Merge branch 'main' into ose_166_scaffolding_for_submission

00b0a41

Merge branch 'main' into ose_166_scaffolding_for_submission

0ad2cbb

Merge branch 'main' into ose_166_scaffolding_for_submission

fdbbd62

decentralgabe reviewed Nov 14, 2022

View reviewed changes

decentralgabe requested changes Nov 14, 2022

View reviewed changes

andresuribe87 commented Nov 14, 2022

View reviewed changes

andresuribe87 added 4 commits November 14, 2022 15:18

spec

8a0a1e5

zero structs

8ddd3c5

Consolidate under the same API

8b0732b

mage spec

f51d248

andresuribe87 and others added 9 commits November 14, 2022 15:18

Minor updates

462be9d

[OSE-167] Scaffolding for /v1/operations (TBD54566975#172)

358e957

* The simplest scaffolding possible * rename Co-authored-by: Gabe <gcohen@squareup.com>

Fix lint and compilation

8feaff6

PR fixes

736fbd6

rename

021e0d4

regen

8b1ec94

Merge branch 'main' into ose_166_scaffolding_for_submission

0fe1736

spec

8ec54cd

Removed unused config

a44b45b

decentralgabe reviewed Nov 14, 2022

View reviewed changes

decentralgabe approved these changes Nov 14, 2022

View reviewed changes

reviews

4ef3ccf

andresuribe87 merged commit 114818b into TBD54566975:main Nov 15, 2022

	resp := GetSubmissionResponse{}
	var resp GetSubmissionResponse

[OSE-166] Scaffolding for /v1/presentations/submissions #171

[OSE-166] Scaffolding for /v1/presentations/submissions #171

Conversation

andresuribe87 commented Nov 11, 2022 • edited Loading

codecov-commenter commented Nov 11, 2022 • edited Loading

Codecov Report

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 left a 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 Nov 11, 2022 •

edited

Loading

codecov-commenter commented Nov 11, 2022 •

edited

Loading