refactor: HTTP client interface #1776

nasdf · 2023-08-11T18:58:23Z

Relevant issue(s)

Description

This PR is split into two to make reviewing easier: #1839

This PR adds an HTTP client and server implementation that implements the client.DB interface and runs through the existing integration test suite.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test

Specify the platform(s) on which this was tested:

MacOS

…t test wrapper to http package. fix tests with missing result fields

codecov · 2023-08-23T14:55:23Z

Codecov Report

Patch coverage: 0.23% and project coverage change: -5.43% ⚠️

Comparison is base (ebad3eb) 75.63% compared to head (170d07a) 70.20%.

@@             Coverage Diff             @@
##           develop    #1776      +/-   ##
===========================================
- Coverage    75.63%   70.20%   -5.43%     
===========================================
  Files          209      225      +16     
  Lines        22261    23987    +1726     
===========================================
+ Hits         16835    16838       +3     
- Misses        4268     5992    +1724     
+ Partials      1158     1157       -1

Flag	Coverage Δ
all-tests	`70.20% <0.23%> (-5.43%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
http/client.go	`0.00% <0.00%> (ø)`
http/client_collection.go	`0.00% <0.00%> (ø)`
http/client_lens.go	`0.00% <0.00%> (ø)`
http/client_tx.go	`0.00% <0.00%> (ø)`
http/errors.go	`0.00% <0.00%> (ø)`
http/handler_collection.go	`0.00% <0.00%> (ø)`
http/handler_lens.go	`0.00% <0.00%> (ø)`
http/handler_store.go	`0.00% <0.00%> (ø)`
http/handler_tx.go	`0.00% <0.00%> (ø)`
http/http_client.go	`0.00% <0.00%> (ø)`
... and 7 more

... and 4 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebad3eb...170d07a. Read the comment docs.

📢 Have feedback on the report? Share it here.

AndrewSisley · 2023-09-05T13:03:16Z

tests/integration/lens.go

@@ -75,9 +75,34 @@ func getMigrations(

 		configs, err := db.LensRegistry().Config(s.ctx)
 		require.NoError(s.t, err)
-
-		// The order of the results is not deterministic, so do not assert on the element


todo: I think this comment still adds value and should be preserved somewhere. It is not obvious reading the code as to why the order of the elements is not asserted.

AndrewSisley · 2023-09-05T13:20:21Z

http/client_lens.go

+		return nil, err
+	}
+	var result enumerable.Enumerable[map[string]any]
+	if err := c.http.requestJson(req, &result); err != nil {


question: Are you 100% sure this works? You appear to be unmarshalling json to an interface. If it does work, how, and what is the resultant concrete type? If it does not work, did the tests miss this, or did you forget to run them? If they missed it, how?

MigrateUp, MigrateDown on LensRegistry and lensHandler appear to do this.

I think I know why this was missed.

Previously we have been able to rely on the fact that we only ever had one implementation of the client interfaces, this allowed us to get away with not directly testing most functions implemented, so long as they were called internally by functions that were directly tested.

These functions (as do most of them, our testing is horrible across the go-embedded stuff) fall into that category. They have no tests targeting them.

I think this requires discussion at standup, as it means that significant chunks of this PR are untested, as will likely much of the CLI stuff be. There is a significant risk of other bugs besides the probable one here that I have flagged.

AndrewSisley · 2023-09-05T13:33:48Z

tests/integration/utils2.go

 	default:
-		return false
+		// Default is to test all client types.
+		if !goClient && !httpClient {


todo: I disagree with this default. It will essentially double the stuff being done when running go test or make test, and will significantly degrade the dev-ex of anyone working in the codebase.

Can you please default to the go client only, and then maybe add a couple of make actions to make it easy to run http?

AndrewSisley · 2023-09-05T13:44:36Z

tests/integration/utils2.go

+	// Set default values for the specified testing mode.
+	switch {
+	case DetectDbChanges:
+		// Change detector runs using only the go client type.


todo: I think the code should change. Whilst we currently think that running the change detector via the http client is pointless, it is not technically impossible and we shouldnt prevent it. The environment variables should still be respected, and it will cause confusion (and bugs) if they are not.

Can you please instead change the make test:changes action to instead explicitly run using just the Go client. Regardless of what the default clients are (see other comment about defaults).

… type

…esults default behavior.

AndrewSisley

Cheers Keenan - looks really good :) I'm approving but, would recommend you quickly check in with another dev too if any are interested as it is a significant change and they may have different opinions to mine (and catch stuff I have missed).

shahzadlone

LGTM but I have some questions I asked and some non-blocking comments / thoughts:

The PR was quite easy to review, however I think it could have been even easier/clearer if:

The testing framework changes could have been isolated in a separate PR.
Rebase flow was used instead of the multiple merges from develop into your branch, which create those non-linear commit history with extra merge commits that aren't useful for reviewers.

CodeCov shows that code coverage has dropped by -5.40%, that is quite a lot, I am not quite sure why that is, I know sometimes it can be inaccurate and show a different number on merge, but usually it is not off by that much. This isn't a blocker but might be worth exploring / looking into it why that is.

shahzadlone · 2023-09-06T21:14:06Z

tests/integration/utils2.go

+	httpClient, _ = strconv.ParseBool(os.Getenv(clientHttpEnvName))
+	goClient, _ = strconv.ParseBool(os.Getenv(clientGoEnvName))
+	badgerFile, _ = strconv.ParseBool(os.Getenv(fileBadgerEnvName))
+	badgerInMemory, _ = strconv.ParseBool(os.Getenv(memoryBadgerEnvName))
+	inMemoryStore, _ = strconv.ParseBool(os.Getenv(inMemoryEnvName))
+	DetectDbChanges, _ = strconv.ParseBool(os.Getenv(detectDbChangesEnvName))
+	SetupOnly, _ = strconv.ParseBool(os.Getenv(setupOnlyEnvName))


question: what made you switch from LookupEnv to Getenv.

IRRC Getenv returns an empty string, even in cases where the environment variable is not set and LookupEnv returns the value and a bool to indicate if it was set or not.

We weren't using the second return value from LookupEnv for the boolean variables. The behavior should be the same but is a bit easier to read in my opinion.

I thought we were using the second value for some of them. However I don't mind this minor change just wanted to know the reason.

shahzadlone · 2023-09-06T21:16:16Z

tests/integration/utils2.go

 		targetBranchValue = "develop"
 	}

-	if mutationTypeSpecified {
-		mutationType = MutationType(mutType)
+	if value, ok := os.LookupEnv(mutationTypeEnvName); ok {


question: What was the benefit to moving the env lookup here? I did like how they were all in one place above.

I think this makes it more obvious that we have a default value for these variables. We also don't have to keep track of extra variables since the lookup return values are now scoped to the if statement.

I think this is subjective, I do like if all the lookups are done in the beginning and then the condition flow is executed, rather than lookup and condition checking happening at the same step. But not a strong pref, so is a non-blocking one.

shahzadlone · 2023-09-06T22:22:58Z

@nasdf I noticed that everything under http/ has 0 coverage, is that expected for now?

nasdf · 2023-09-07T17:37:36Z

@nasdf I noticed that everything under http/ has 0 coverage, is that expected for now?

I think it's not picking up the coverage from the integration tests correctly.

AndrewSisley · 2023-09-07T17:41:10Z

I think it's not picking up the coverage from the integration tests correctly.

It wont do, this is expected, as the codecov job will only be running with the go client for now. We need to come up with a performant way of handling this, for both the multi-clients, the mutations, and change detector - I think we spoke about this in a recent standup.

shahzadlone · 2023-09-08T02:03:00Z

Sorry I had a small subjective nitpick I forgot to mention, regarding some function names (if these are added by you and not existent from before). I do like boolean returning functions to start with is or are to make it clear that a "question"/"equality" is being "answered"/"compared", for example:

resultArraysAreEqual
resultOptionsAreEqual
resultsAreEqual
resultsAreAnyOf

Could be:

areResultArraysEqual
areResultOptionsEqual
areResultsEqual
areResultsAnyOf

shahzadlone

Posted some nitpicks(non-blocking). All G otherwise

## Relevant issue(s) Closes #1472 Closes #1507 Closes #1860 ## Description This is a follow up to #1776 This PR adds a CLI implementation that implements the client.DB interface and runs through the existing integration test suite. - [x] Merge existing server config code - [x] Refactor CLI to use new HTTP client - [x] Remove `net/api` package - [x] Remove `api/http` package - [x] Lens tests are timing out in CI: fixed #1862 - [x] Code coverage is incorrectly reporting: fixed #1861 - [x] Flaky test causing failures: fixed #1912 Renamed Commands: - `peerid` to `client peer info` - `client p2pcollection` to `client p2p collection` - `client replicator` to `client p2p replicator` - `client schema list` to `client collection describe` Removed Commands: - `block get` - `ping` - `rpc` Added Commands: - `client collection create` - `client collection delete` - `client collection get` - `client collection keys` - `client collection update` - `client tx create` - `client tx discard` - `client tx commit` - `client schema migration up` - `client schema migration down` - `client schema migration reload` **Notes for reviewers**: - `.github` changes are merged from #1871 - `Makefile` most of these changes are also from #1871 - `docs/cli` ignore these changes, it will be updated next release - sorry for all of the merge commits, I am working on learning rebase flow ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? `make test` Specify the platform(s) on which this was tested: - MacOS

## Relevant issue(s) ## Description This PR is split into two to make reviewing easier: sourcenetwork#1839 This PR adds an HTTP client and server implementation that implements the `client.DB` interface and runs through the existing integration test suite. ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? `make test` Specify the platform(s) on which this was tested: - MacOS

## Relevant issue(s) Closes sourcenetwork#1472 Closes sourcenetwork#1507 Closes sourcenetwork#1860 ## Description This is a follow up to sourcenetwork#1776 This PR adds a CLI implementation that implements the client.DB interface and runs through the existing integration test suite. - [x] Merge existing server config code - [x] Refactor CLI to use new HTTP client - [x] Remove `net/api` package - [x] Remove `api/http` package - [x] Lens tests are timing out in CI: fixed sourcenetwork#1862 - [x] Code coverage is incorrectly reporting: fixed sourcenetwork#1861 - [x] Flaky test causing failures: fixed sourcenetwork#1912 Renamed Commands: - `peerid` to `client peer info` - `client p2pcollection` to `client p2p collection` - `client replicator` to `client p2p replicator` - `client schema list` to `client collection describe` Removed Commands: - `block get` - `ping` - `rpc` Added Commands: - `client collection create` - `client collection delete` - `client collection get` - `client collection keys` - `client collection update` - `client tx create` - `client tx discard` - `client tx commit` - `client schema migration up` - `client schema migration down` - `client schema migration reload` **Notes for reviewers**: - `.github` changes are merged from sourcenetwork#1871 - `Makefile` most of these changes are also from sourcenetwork#1871 - `docs/cli` ignore these changes, it will be updated next release - sorry for all of the merge commits, I am working on learning rebase flow ## Tasks - [x] I made sure the code is well commented, particularly hard-to-understand areas. - [x] I made sure the repository-held documentation is changed accordingly. - [x] I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in [tools/configs/chglog/config.yml](tools/configs/chglog/config.yml)). - [x] I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ... ## How has this been tested? `make test` Specify the platform(s) on which this was tested: - MacOS

nasdf added 22 commits August 10, 2023 17:11

initial version of http server and client

a8d6791

update lens client http base url

25d2250

integration tests working with http client

1e70947

fix linter errors

466fe78

fixes to make more tests pass

d86ee90

add http subscription logic

ef58ecd

implement more http collection methods

da866b5

implement lens http server and client

1c3c828

refactor http client and server for better use in testing. move clien…

6a8e798

…t test wrapper to http package. fix tests with missing result fields

implement http transaction api

0162b1c

http client fixes

38d0f0e

clean document after save and update

7f00cba

more http client test fixes

9962325

almost all tests passing

f5c29b3

fix lint errors

6a72bb6

Merge branch 'develop' into nasdf/refactor/api-client-interface

ce0054a

enable all tests

508cd83

fix http lens client with incorrect method path

0ebf11c

fix http transaction middleware

677f18f

handle transaction conflict errors in http client

48a4103

update lens getMigrations test logic

41ce1cd

fix linter warnings

edd246e

nasdf added 7 commits August 24, 2023 10:24

fix race in event publish

181d6c3

add wait group sync to publisher

680efcd

fix bug in memory datastore

b6f7d4a

fix memory transaction deadlock

166080e

use transaction close lock in memory datastore

509cf00

revert publisher changes

add612f

disable lens transactions in http middleware

b8614a3

nasdf added 4 commits September 1, 2023 11:22

test utils cleanup. fix change detector defaults

9f88fef

add comments to ignored close errors in http client

7b12e04

add http middleware context documentation

7f7f747

relax comparisons when testing non go clients

5f96d78

AndrewSisley reviewed Sep 5, 2023

View reviewed changes

nasdf added 4 commits September 5, 2023 08:51

add error documentation. restore lens test comment

31f9b98

only test go client by default. allow change detector with any client…

cba08de

… type

clean up generic function calls. remove comment notes. adjust assertR…

4dded7d

…esults default behavior.

fix lens http client parsing bug

df79ef4

AndrewSisley approved these changes Sep 5, 2023

View reviewed changes

nasdf added 2 commits September 6, 2023 08:13

fix bug with body parsing logic in lens migrate handlers

05f6f80

fix bug in http lens client migrate

97efa00

shahzadlone reviewed Sep 6, 2023

View reviewed changes

Merge branch 'develop' into nasdf/refactor/api-client-interface

35d0040

shahzadlone approved these changes Sep 8, 2023

View reviewed changes

nasdf added 2 commits September 8, 2023 08:18

fixes for failing http tests

cc773c7

update result test helper names

170d07a

nasdf merged commit a8c253b into sourcenetwork:develop Sep 8, 2023
14 checks passed

AndrewSisley added this to the DefraDB v0.7 milestone Sep 11, 2023

AndrewSisley assigned nasdf Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: HTTP client interface #1776

refactor: HTTP client interface #1776

nasdf commented Aug 11, 2023 •

edited

codecov bot commented Aug 23, 2023 •

edited

AndrewSisley Sep 5, 2023

AndrewSisley Sep 5, 2023

AndrewSisley Sep 5, 2023

AndrewSisley Sep 5, 2023

AndrewSisley Sep 5, 2023

AndrewSisley left a comment

shahzadlone left a comment

shahzadlone Sep 6, 2023

nasdf Sep 7, 2023

shahzadlone Sep 8, 2023

shahzadlone Sep 6, 2023

nasdf Sep 7, 2023

shahzadlone Sep 8, 2023

shahzadlone commented Sep 6, 2023

nasdf commented Sep 7, 2023

AndrewSisley commented Sep 7, 2023

shahzadlone commented Sep 8, 2023

shahzadlone left a comment

refactor: HTTP client interface #1776

refactor: HTTP client interface #1776

Conversation

nasdf commented Aug 11, 2023 • edited

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Aug 23, 2023 • edited

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

AndrewSisley left a comment

Choose a reason for hiding this comment

shahzadlone 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

shahzadlone commented Sep 6, 2023

nasdf commented Sep 7, 2023

AndrewSisley commented Sep 7, 2023

shahzadlone commented Sep 8, 2023

shahzadlone left a comment

Choose a reason for hiding this comment

nasdf commented Aug 11, 2023 •

edited

codecov bot commented Aug 23, 2023 •

edited