refactor: Change handler functions implementation and response formatting #498

fredcarle · 2022-06-02T21:21:36Z

RELEVANT ISSUE(S)

Resolves #384
Resolves #458
Resolves #494

DESCRIPTION

After the HTTP API refactor, we now focus on the refactoring of the handler functions themselves ensuring common response formats for both successful and error responses across the API.

This PR also changes the API version number to v0 from v1 to show "unstable" status and responds with the appropriate content-type with JSON payloads.

HOW HAS THIS BEEN TESTED?

These changes have been unit tested.

CHECKLIST:

I have commented the code, particularly in hard-to-understand areas.
I have made corresponding changes to the repo-held documentation.
I have made sure that the PR title adheres to the conventional commit style (subset of the ones we use can be found under: tools/configs/chglog/config.yml

ENVIRONMENT / OS THIS WAS TESTED ON?

Please specify which of the following was this tested on (remove or add your own):

Arch Linux
Debian Linux
MacOS
Windows

AndrewSisley

Looks good! Code is much nicer IMO - just got the one comment on one func which I think could be made nicer still. Would also suggest commiting with a lightly more specific message than the current PR title as it sounds a be vague to me atm (maybe something like 'Refactor handler response formatting', and then just note the API v change in the commit body?).

Approved assuming you respond sensibly to my one comment pre-merge :)

api/http/handler.go

fredcarle · 2022-06-03T02:03:12Z

@AndrewSisley let me know if you like the changed PR title better

AndrewSisley · 2022-06-03T14:01:04Z

@AndrewSisley let me know if you like the changed PR title better

Is good - cheers Fred!

orpheuslummis

Approved conditional on typo and ioutil issue being fixed.

api/http/handlerfuncs.go

api/http/errors_test.go

api/http/handlerfuncs_test.go

orpheuslummis · 2022-06-06T16:26:11Z

api/http/handlerfuncs.go

+		rw,
+		dataResponse{map[string]interface{}{"result": "success"}},
+		http.StatusBadRequest,
+	)
 }

 func getBlockHandler(rw http.ResponseWriter, req *http.Request) {


suggestion(non-blocking): there could be tests for the following in getBlockHandler (lack of them can be seen via code coverage):

dag.DecodeProtobuf(block.RawData()) failing

buf, err := nd.MarshalJSON() failing

delta, err := reg.DeltaDecode(nd) failing

data, err := delta.Marshal() failing

If you can give me an example of a request that would make them fail individually I would gladly add it. They are the only 4 untested blocks in the package because I couldn't find a way to make them fail.

Sorry, my comment was not thought out. I don't have such an example.

api/http/handlerfuncs.go

shahzadlone

Beautiful Work! Super happy with the testing, as the coverage drop that we had in 593214a comes back up 💪 significantly in this PR.

Mr. @orpheuslummis beat me to my concerns with ioutil stuff. But approving for now assuming that will be fixed 😄

jsimnz

LGTM, minor thought added.

nitpick: Technically this PR covers 3 different bug, feature, and refactor issues making it somewhat difficult to accurately assign a PR prefix, between the three. At the moment this has a refactor prefix, which is prob the best since that is the most notable section of the change, and the others are smaller.

Just has the negative consequence that when we generate the changelog the bugfix and feature won't be in the list explicitly.

jsimnz · 2022-06-07T20:07:37Z

api/http/handler.go

+// Odd arguments are the keys and must be strings otherwise they are ignored.
+// Even arguments are the values associated with the previous key.
+// Odd arguments are also ignored if there are no following arguments.
+func simpleDataResponse(args ...interface{}) dataResponse {


thought: Since this is an internal func, im not too worried about it, but just wondering in case a dev shoots themselves in the foot and have an un-even number of func args so the keys/values don't line up.

If it's an odd number of arguments, the last one is discarded as the function documentation says. They will then notice that some information is missing in the payload which will prompt them to look at the data sent to the sendJSON function.

fredcarle · 2022-06-08T03:29:17Z

LGTM, minor thought added.

nitpick: Technically this PR covers 3 different bug, feature, and refactor issues making it somewhat difficult to accurately assign a PR prefix, between the three. At the moment this has a refactor prefix, which is prob the best since that is the most notable section of the change, and the others are smaller.

Just has the negative consequence that when we generate the changelog the bugfix and feature won't be in the list explicitly.

That's usually a pretty common situation with refactors no? Otherwise it would have made for pretty incomplete dependant PRs.

codecov · 2022-06-08T13:18:56Z

Codecov Report

Merging #498 (ae3eb52) into develop (d200cb2) will increase coverage by 1.54%.
The diff coverage is 94.36%.

@@             Coverage Diff             @@
##           develop     #498      +/-   ##
===========================================
+ Coverage    52.55%   54.10%   +1.54%     
===========================================
  Files           97       97              
  Lines        13170    13099      -71     
===========================================
+ Hits          6922     7087     +165     
+ Misses        5574     5337     -237     
- Partials       674      675       +1

Impacted Files	Coverage Δ
api/http/router.go	`100.00% <ø> (ø)`
api/http/handlerfuncs.go	`89.67% <91.66%> (+88.86%)`	⬆️
api/http/errors.go	`100.00% <100.00%> (ø)`
api/http/handler.go	`100.00% <100.00%> (ø)`
query/graphql/schema/generate.go	`82.40% <0.00%> (+0.37%)`	⬆️
core/crdt/lwwreg.go	`74.39% <0.00%> (+2.43%)`	⬆️
db/schema.go	`41.46% <0.00%> (+7.31%)`	⬆️

jsimnz · 2022-06-08T13:50:38Z

LGTM, minor thought added.
nitpick: Technically this PR covers 3 different bug, feature, and refactor issues making it somewhat difficult to accurately assign a PR prefix, between the three. At the moment this has a refactor prefix, which is prob the best since that is the most notable section of the change, and the others are smaller.
Just has the negative consequence that when we generate the changelog the bugfix and feature won't be in the list explicitly.

That's usually a pretty common situation with refactors no? Otherwise it would have made for pretty incomplete dependant PRs.

Its common enough to have a bug+refactor together, but the feature issue was pretty unrelated. Doesn't matter in the grand scheme of things, which is why it was labeled as a nitpick :)

jsimnz · 2022-06-08T13:51:07Z

Unrelated, as in not dependant, not unrelated to the section of changes.**

AndrewSisley · 2022-06-08T14:00:28Z

That's usually a pretty common situation with refactors no? Otherwise it would have made for pretty incomplete dependant PRs.

I really dont want to discourage refactoring in anyway - I think ideally yes it would be nice if they were all split out into different PRs, but that is not always practical, and if we start asking people to do this it may (slightly) discourage refactoring leading to a slightly less nice code base

fredcarle · 2022-06-08T14:21:50Z

Unrelated, as in not dependant, not unrelated to the section of changes.**

I'm curious as to which feature you are talking about.

jsimnz · 2022-06-08T14:36:50Z

This is only in reference to the 3 linked issues. #384 refactor, #458 bug, and #494 feature.

Obviously the refactor here spans a lot of different aspects/potential bug fixes/features just to resolve #384. In addition to the various changes needed for 384. This PR also implements the feature of #494.

I'm all for adding whatever is needed to implement a given task, and resolving multiple tickets within a single PR, just a nitpick from the perspective of the changelog only for the independant change of #494.

fredcarle · 2022-06-08T14:42:53Z

@jsimnz Ah now I get it 🙂. I wasn't thinking of #494 as a feature but I see what you mean. I thought it made sense to do that here cuz it was such a small change and related to the HTTP API. If you prefer those kinds of changes be done in their own PR in the future, I'll make sure to do so.

jsimnz · 2022-06-08T14:46:57Z

Exactly, due to the size (1 liner) of the change, its completely reasonable to throw it in here :).

…ting (sourcenetwork#498) RELEVANT ISSUE(S) Resolves sourcenetwork#384 Resolves sourcenetwork#458 Resolves sourcenetwork#494 DESCRIPTION After the HTTP API refactor, we now focus on the refactoring of the handler functions themselves ensuring common response formats for both successful and error responses across the API. This PR also changes the API version number to v0 from v1 to show "unstable" status and responds with the appropriate content-type with JSON payloads.

fredcarle added area/api Related to the external API component refactor This issue specific to or requires *notable* refactoring of existing codebases and components action/no-benchmark Skips the action that runs the benchmark. labels Jun 2, 2022

fredcarle added this to the DefraDB v0.3 milestone Jun 2, 2022

fredcarle requested a review from a team June 2, 2022 21:21

AndrewSisley approved these changes Jun 3, 2022

View reviewed changes

api/http/handler.go Outdated Show resolved Hide resolved

fredcarle changed the title ~~refactor: Change handler function implementations~~ refactor: Change handler functions implementation and response formatting Jun 3, 2022

fredcarle self-assigned this Jun 3, 2022

orpheuslummis approved these changes Jun 6, 2022

View reviewed changes

shahzadlone approved these changes Jun 6, 2022

View reviewed changes

fredcarle force-pushed the fredcarle/refactor/I384-http-api-handlerfuncs branch from 8bab889 to 418cb1a Compare June 6, 2022 21:25

jsimnz approved these changes Jun 7, 2022

View reviewed changes

fredcarle force-pushed the fredcarle/refactor/I384-http-api-handlerfuncs branch from 7722344 to e04a008 Compare June 8, 2022 03:05

fredcarle added 8 commits June 8, 2022 09:10

refactor handlerfuncs

6a055e3

change /api/v1 to /api/v2

e73aa86

remove switch in sendJSON

d4b24c1

add dataResponse helper function

7eefc6e

fix typos

26d2272

change error message and multiline raw string formatting

bce42de

change blocks path

23bfc20

fix newHandler call according to CORS feature

ae3eb52

fredcarle force-pushed the fredcarle/refactor/I384-http-api-handlerfuncs branch from e04a008 to ae3eb52 Compare June 8, 2022 13:13

fredcarle merged commit b8beabb into develop Jun 8, 2022

fredcarle deleted the fredcarle/refactor/I384-http-api-handlerfuncs branch June 8, 2022 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Change handler functions implementation and response formatting #498

refactor: Change handler functions implementation and response formatting #498

fredcarle commented Jun 2, 2022 •

edited

Loading

AndrewSisley left a comment

fredcarle commented Jun 3, 2022

AndrewSisley commented Jun 3, 2022

orpheuslummis left a comment

orpheuslummis Jun 6, 2022

fredcarle Jun 6, 2022

orpheuslummis Jun 7, 2022

shahzadlone left a comment

jsimnz left a comment

jsimnz Jun 7, 2022

fredcarle Jun 8, 2022

fredcarle commented Jun 8, 2022

codecov bot commented Jun 8, 2022 •

edited

Loading

jsimnz commented Jun 8, 2022

jsimnz commented Jun 8, 2022

AndrewSisley commented Jun 8, 2022

fredcarle commented Jun 8, 2022

jsimnz commented Jun 8, 2022

fredcarle commented Jun 8, 2022

jsimnz commented Jun 8, 2022

refactor: Change handler functions implementation and response formatting #498

refactor: Change handler functions implementation and response formatting #498

Conversation

fredcarle commented Jun 2, 2022 • edited Loading

RELEVANT ISSUE(S)

DESCRIPTION

HOW HAS THIS BEEN TESTED?

CHECKLIST:

ENVIRONMENT / OS THIS WAS TESTED ON?

AndrewSisley left a comment

Choose a reason for hiding this comment

fredcarle commented Jun 3, 2022

AndrewSisley commented Jun 3, 2022

orpheuslummis left a comment

Choose a reason for hiding this comment

orpheuslummis Jun 6, 2022

Choose a reason for hiding this comment

fredcarle Jun 6, 2022

Choose a reason for hiding this comment

orpheuslummis Jun 7, 2022

Choose a reason for hiding this comment

shahzadlone left a comment

Choose a reason for hiding this comment

jsimnz left a comment

Choose a reason for hiding this comment

jsimnz Jun 7, 2022

Choose a reason for hiding this comment

fredcarle Jun 8, 2022

Choose a reason for hiding this comment

fredcarle commented Jun 8, 2022

codecov bot commented Jun 8, 2022 • edited Loading

Codecov Report

jsimnz commented Jun 8, 2022

jsimnz commented Jun 8, 2022

AndrewSisley commented Jun 8, 2022

fredcarle commented Jun 8, 2022

jsimnz commented Jun 8, 2022

fredcarle commented Jun 8, 2022

jsimnz commented Jun 8, 2022

fredcarle commented Jun 2, 2022 •

edited

Loading

codecov bot commented Jun 8, 2022 •

edited

Loading