fix(dp): change response from 500 to 409 when existing serial number modified #12372

WojSad · 2022-04-01T12:13:47Z

Signed-off-by: Wojciech Sadowy wojciech.sadowy@freedomfi.com

When a CreateCbsd request was issued to backend with a serial_number of an existing cbsd, (Internal server error) 500 error was returned. The same was true for updating a cbsd with a serial number of an already existing serial number. Changed the error to Conflict (409).

Summary

Modified handlers.go

Test Plan

Integration tests added

github-actions · 2022-04-01T12:16:30Z

Thanks for opening a PR! 💯

A couple initial guidelines

All commits must be signed off. This is enforced by DCO check.
All PR titles must follow the semantic commits format. This is enforced by Semantic PR.

Howto

Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
Checks. All required CI checks must pass before merge.
Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

Contributing Conventions for norms around contributed code

If this is your first Magma PR, also consider reading

Developer Onboarding for onboarding as a new Magma developer
Development Workflow for guidance on your first pull request
CI Checks for points of contact for failing or flaky CI checks
GitHub-to-Slack mappings for Magma maintainers for guidance on how to contact maintainers on Slack

github-actions · 2022-04-01T12:18:48Z

Oops! Looks like you failed the Semantic PR check.

Howto

Instructions on formatting your PR title
For PRs with only one commit, the commit message must also be semantic. See Changing a commit message for a howto
Guide to the different CI checks and resolution guidelines

♻️ Updated: ✅ The check is passing the Semantic PR after the last commit.

jkmar · 2022-04-01T12:32:04Z

dp/cloud/go/services/dp/obsidian/cbsd/handlers.go

@@ -216,6 +218,9 @@ func getHttpError(err error) error {
 	case codes.NotFound:
 		return echo.NewHTTPError(http.StatusNotFound, err)
 	default:
+		if strings.Contains(fmt.Sprintf("%s", err), duplicateRecordMsg) == true {


I think we should not rely on error message (which can depend on database implementation), but instead return proper response code, by the servicers codes.AlreadyExists especially, since handlers do not talk directly to the database and are not supposed to understand it.
So I would suggest updating also servicers (return proper error code codes.AlreadyExists)
and db handlers (return proper error merrors.ErrAlreadyExists)

github-actions · 2022-04-06T10:08:46Z

Oops! Looks like you failed the DCO check. Be sure to sign all your commits.

Howto

♻️ Updated: ✅ The check is passing the DCO check after the last commit.

github-actions · 2022-04-06T10:24:34Z

feg-workflow

    2 files 202 suites 38s ⏱️
370 tests 370 ✔️ 0 💤 0 ❌
384 runs 384 ✔️ 0 💤 0 ❌

Results for commit b18d618.

♻️ This comment has been updated with latest results.

jkmar

Approving, just few small comments

jkmar · 2022-04-06T10:18:31Z

dp/cloud/python/magma/test_runner/tests/test_dp_with_orc8r.py

+
+        cbsd1_payload = builder.build_post_data()
+        cbsd2_payload = builder.build_post_data()
+        cbsd1_payload["serial_number"] = "foo"


How about adding this to the builder (with_serial_number)?

jkmar · 2022-04-06T10:22:05Z

orc8r/cloud/go/sqorc/error_checker.go

@@ -0,0 +1,40 @@
+package sqorc


it would be nice to have some tests for this file, since it's a library code

jkmar · 2022-04-06T10:22:34Z

orc8r/cloud/go/sqorc/builder.go

@@ -50,6 +52,25 @@ func GetSqlBuilder() StatementBuilder {
 	}
 }

+// GetErrorChecker returns a squirrel Builder for the configured SQL dialect as
+// found in the SQL_DIALECT env var.
+func GetErrorChecker() ErrorChecker {


Maybe move this to error checker file

jkmar · 2022-04-06T10:29:56Z

dp/cloud/go/services/dp/storage/db/table.go

@@ -43,6 +43,10 @@ func addColumns(builder sqorc.CreateTableBuilder, fields FieldMap) sqorc.CreateT
 			colBuilder = colBuilder.Default(field.DefaultValue)
 		}
 		builder = colBuilder.EndColumn()
+
+		if field.Unique {


Does this work when there are multiple unique columns? Maybe add one simple test to query_test.go for it

jkmar · 2022-04-06T10:30:43Z

dp/cloud/go/services/dp/obsidian/cbsd/handlers.go

@@ -217,6 +217,8 @@ func getHttpError(err error) error {
 	switch s, _ := status.FromError(err); s.Code() {
 	case codes.NotFound:
 		return echo.NewHTTPError(http.StatusNotFound, err)
+	case codes.AlreadyExists:


Maybe one small test for handlers

jkmar · 2022-04-06T10:31:06Z

dp/cloud/go/services/dp/servicers/cbsd_manager.go

@@ -166,6 +166,8 @@ func makeErr(err error, wrap string) error {
 	code := codes.Internal
 	if err == merrors.ErrNotFound {
 		code = codes.NotFound
+	} else if err == merrors.ErrAlreadyExists {


Maybe one small test for servicers

github-actions · 2022-04-06T10:46:59Z

cloud-workflow

    7 files 352 suites 2m 8s ⏱️
966 tests 966 ✔️ 0 💤 0 ❌

Results for commit b18d618.

♻️ This comment has been updated with latest results.

github-actions · 2022-04-06T11:01:30Z

agw-workflow

    77 files   122 suites 7m 23s ⏱️
1 134 tests 1 125 ✔️ 9 💤 0 ❌
1 135 runs 1 126 ✔️ 9 💤 0 ❌

Results for commit b18d618.

♻️ This comment has been updated with latest results.

WojSad · 2022-04-08T09:10:41Z

@magma/approvers-orc8r changes ready to merge, please have a look

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

github-actions · 2022-04-08T22:59:28Z

Unit Test Results

157 files 157 suites 2h 3m 4s ⏱️
308 tests 305 ✔️ 3 💤 0 ❌

Results for commit 5bca245.

♻️ This comment has been updated with latest results.

…modified (magma#12372) * fix(dp): return 409 in CRUD when cbsd serial number exists Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com> * add more unit tests Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com> * add setup and teardown to error checker tests Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com> * integration test fix Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com> Co-authored-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

WojSad requested review from a team and sklgromek April 1, 2022 12:13

pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines. label Apr 1, 2022

jkmar requested changes Apr 1, 2022

View reviewed changes

WojSad force-pushed the crud-response-status-fix branch 2 times, most recently from be2d1de to e7bf1f1 Compare April 6, 2022 10:07

WojSad requested review from a team and hcgatewood April 6, 2022 10:07

pull-request-size bot added size/L Denotes a Pull Request that changes 100-499 lines. and removed size/M Denotes a PR that changes 30-99 lines. labels Apr 6, 2022

github-actions bot added the component: orc8r Orchestrator-related issue label Apr 6, 2022

WojSad force-pushed the crud-response-status-fix branch from e7bf1f1 to ef4f85c Compare April 6, 2022 10:11

jkmar approved these changes Apr 6, 2022

View reviewed changes

WojSad force-pushed the crud-response-status-fix branch 2 times, most recently from e428c9b to 981e14c Compare April 7, 2022 10:42

WojSad changed the title ~~fix(dp): change response from 500 to 400 when existing serial number modified~~ fix(dp): change response from 500 to 409 when existing serial number modified Apr 7, 2022

themarwhal approved these changes Apr 8, 2022

View reviewed changes

Wojciech Sadowy added 3 commits April 8, 2022 15:38

fix(dp): return 409 in CRUD when cbsd serial number exists

bcd7265

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

add more unit tests

0c84415

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

add setup and teardown to error checker tests

45866b7

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

WojSad force-pushed the crud-response-status-fix branch from afcc5f1 to 45866b7 Compare April 8, 2022 13:38

integration test fix

b18d618

Signed-off-by: Wojciech Sadowy <wojciech.sadowy@freedomfi.com>

WojSad merged commit 5bca245 into magma:master Apr 8, 2022

WojSad deleted the crud-response-status-fix branch May 25, 2022 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dp): change response from 500 to 409 when existing serial number modified #12372

fix(dp): change response from 500 to 409 when existing serial number modified #12372

WojSad commented Apr 1, 2022 •

edited

github-actions bot commented Apr 1, 2022

github-actions bot commented Apr 1, 2022 •

edited

jkmar Apr 1, 2022

github-actions bot commented Apr 6, 2022 •

edited

github-actions bot commented Apr 6, 2022 •

edited

jkmar left a comment

jkmar Apr 6, 2022

WojSad Apr 6, 2022

jkmar Apr 6, 2022

WojSad Apr 6, 2022

jkmar Apr 6, 2022

WojSad Apr 6, 2022

jkmar Apr 6, 2022

WojSad Apr 7, 2022

jkmar Apr 6, 2022

WojSad Apr 7, 2022

jkmar Apr 6, 2022

WojSad Apr 7, 2022

github-actions bot commented Apr 6, 2022 •

edited

github-actions bot commented Apr 6, 2022 •

edited

WojSad commented Apr 8, 2022

github-actions bot commented Apr 8, 2022 •

edited

fix(dp): change response from 500 to 409 when existing serial number modified #12372

fix(dp): change response from 500 to 409 when existing serial number modified #12372

Conversation

WojSad commented Apr 1, 2022 • edited

Summary

Test Plan

github-actions bot commented Apr 1, 2022

Howto

More info

github-actions bot commented Apr 1, 2022 • edited

Howto

Choose a reason for hiding this comment

github-actions bot commented Apr 6, 2022 • edited

Howto

github-actions bot commented Apr 6, 2022 • edited

feg-workflow

jkmar 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

github-actions bot commented Apr 6, 2022 • edited

cloud-workflow

github-actions bot commented Apr 6, 2022 • edited

agw-workflow

WojSad commented Apr 8, 2022

github-actions bot commented Apr 8, 2022 • edited

Unit Test Results

WojSad commented Apr 1, 2022 •

edited

github-actions bot commented Apr 1, 2022 •

edited

github-actions bot commented Apr 6, 2022 •

edited

github-actions bot commented Apr 6, 2022 •

edited

github-actions bot commented Apr 6, 2022 •

edited

github-actions bot commented Apr 6, 2022 •

edited

github-actions bot commented Apr 8, 2022 •

edited