refactor: CLI client interface #1839

nasdf · 2023-08-31T19:15:10Z

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.

Merge existing server config code
Refactor CLI to use new HTTP client
Remove net/api package
Remove api/http package
Lens tests are timing out in CI: fixed ci: Test matrix #1862
Code coverage is incorrectly reporting: fixed test(i): Do not review #1861
Flaky test causing failures: fixed fix(i): Flaky normalize filter test #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 ci: Parallelize change detector #1871
Makefile most of these changes are also from ci: Parallelize change detector #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

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

AndrewSisley

I'm only part way through my review, but I thought I'd submit the comments I have so far. Overall it looks really good, and it is a fantastic amount of code you've been able to remove, whilst gaining features :)

AndrewSisley · 2023-09-29T14:24:33Z

cli/backup_export.go

-		RunE: func(cmd *cobra.Command, args []string) (err error) {
+		Args: cobra.ExactArgs(1),
+		RunE: func(cmd *cobra.Command, args []string) error {
+			store := cmd.Context().Value(storeContextKey).(client.Store)


suggestion: You do cmd.Context().Value(storeContextKey).(client.Store) quite a lot, it might be worth hiding the const and the cast behind a func (perhaps located next to the func that sets it).

AndrewSisley · 2023-09-29T14:30:14Z

cli/blocks_get.go

-	"github.com/sourcenetwork/defradb/config"
-)
-
-func MakeBlocksGetCommand(cfg *config.Config) *cobra.Command {


todo: It might be handy for reviewers (and yourself) to list the commands that have been totally removed without replacement, such as this one. It would make it easier to spot and then talk about them.

I am pretty happy losing functionality in this PR, but medium-long term we may want some of them back via the client interfaces.

I've added a list of the new, renamed, and removed commands to the description.

Cheers Keenan!

AndrewSisley · 2023-09-29T14:31:59Z

cli/p2p_replicator_getall.go

+	"github.com/sourcenetwork/defradb/client"
+)
+
+func MakeP2PReplicatorGetallCommand() *cobra.Command {


nitpick: The a in all should really be capitalised.

AndrewSisley · 2023-09-29T14:33:14Z

cli/p2p_replicator_getall.go

@@ -0,0 +1,36 @@
+// Copyright 2022 Democratized Data Foundation


todo: I hate that we have to do this, and am personally happy with being a bit sloppy here, but I know that others are not - this (and any others) should be 2023.

AndrewSisley · 2023-09-29T14:33:56Z

cli/p2p_collection_getall.go

 )

-func MakeP2PCollectionGetallCommand(cfg *config.Config) *cobra.Command {
+func MakeP2PCollectionGetallCommand() *cobra.Command {


nitpick: The a in all should really be capitalised.

AndrewSisley · 2023-09-29T14:48:12Z

cli/document_create.go

+	var collection string
+	var file string
+	var cmd = &cobra.Command{
+		Use:   "create --collection <collection> <document>",


suggestion: <collection> is ambiguous here and it is not clear (outside of Long) whether this means collection ID, collection name, or a collection json blob. Suggest changing to <collectionName>, if this behaviour is kept (read other comments RE structure first maybe).

AndrewSisley · 2023-09-29T14:53:38Z

cli/document_create.go

+		Long: `Create a new document.
+
+Example: create document
+  defradb client document create --collection User '{ "name": "Bob" }'


todo: Here the cli now differs in structure to the client API. Here document is a sibling to collection, despite internally using collection commands.

I think it would reduce the cognitive load on the user if all clients followed the same structure, and I thought this was one of the goals of this block of work.

You are also limiting the user to the default schema version here, if you fix the structure you should get more complex collection selection for free. This also is breaking from the client API.

It might be worth creating a discord thread to discuss this, will leave that up to you though.

I've reorganized this so it better matches the client API.

AndrewSisley · 2023-09-29T14:56:13Z

cli/document_create.go

+					return err
+				}
+				return col.Create(cmd.Context(), doc)
+			case []any:


question: This is quite a lot of code not covered by tests, and the PR description says your only testing was via the make test command, is the PR description incorrect, or have you manually tested these blocks?

suggestion: Write some tests :)

The same question/suggestion applies to quite a few commands in this PR.

I think adding more integration tests would be the best option. I will put together a list of areas that are missing coverage.

AndrewSisley · 2023-09-29T14:58:14Z

cli/document_create.go

+				}
+				return col.CreateMany(cmd.Context(), docs)
+			default:
+				return fmt.Errorf("invalid document")


todo: This, and any other error string should be in an errors.go file. Happy to expand on why I think so if you need (I cant remember how much you know about this 'standard' :)).

AndrewSisley · 2023-09-29T15:07:59Z

cli/wrapper.go

+
+var _ client.DB = (*Wrapper)(nil)
+
+type Wrapper struct {


question: Why is this production code?

I'd like to move this and the http wrapper to the integration tests package. I think there was something blocking me from doing that, but I will give it another look.

Cheers Keenan - please give me a shout here once you've pushed changes/decided against moving and I'll review them fully.

Update: I was able to move them to a new tests/clients package.

AndrewSisley · 2023-09-29T15:14:54Z

cli/document_keys.go

+				if docKey.Err != nil {
+					results.Error = docKey.Err.Error()
+				}
+				writeJSON(cmd, results) //nolint:errcheck


todo: Please document why this is okay

I decided to return the error here instead.

AndrewSisley · 2023-09-29T15:20:11Z

cli/p2p_replicator_delete.go

+		Short: "Delete a replicator. It will stop synchronizing",
+		Long:  `Delete a replicator. It will stop synchronizing.`,
+		Args:  cobra.ExactArgs(1),
+		RunE: func(cmd *cobra.Command, args []string) error {


todo: This entire command appears to be untested, please test it.

I'd like for this to be covered in an integration test so we can get coverage on other implementations as well. I'll follow up shortly with a fix.

AndrewSisley · 2023-09-29T15:24:42Z

cli/p2p_replicator_set.go

+func MakeP2PReplicatorSetCommand() *cobra.Command {
+	var collections []string
+	var cmd = &cobra.Command{
+		Use:   "set [-c, --collection] <peer>",


todo: [-c, --collection] appears to be inconsistent with the rest of the CLI, I think it should be consistent.

Are you referring to the naming or that it's a slice type?

I forgot that this was a slice type, but I was referring to the existence of the shorthand. However the way you have moved the document stuff into collection has removed the inconsistency (as there is no longer a --collection flag there).

This comment is now no longer relevant :)

AndrewSisley · 2023-09-29T15:29:55Z

cli/peerid.go

-	"github.com/sourcenetwork/defradb/errors"
-)
-
-func MakePeerIDCommand(cfg *config.Config) *cobra.Command {


question: Is this now provided by p2p_info?

Yes, as a follow up I will be adding the multiaddresses as a return value there as well.

AndrewSisley · 2023-09-29T15:35:26Z

cli/schema_migration_down.go

+	var schemaVersionID string
+	var cmd = &cobra.Command{
+		Use:   "down --version <version> <src>",
+		Short: "Reverse a migration on the specified schema version.",


todo: Please explain to the user what <src> is :)

suggestion: on is somewhat ambiguous here I think, from (or to, I cant remember which way round) might be better.

Same goes for the up command.

I tried to describe it in more detail. I believe it is the list of documents to apply / reverse the migration to / from.

cheers Keenan - looks good

AndrewSisley · 2023-10-02T13:04:12Z

cli/collection.go

+		Use:   "collection [--name <name> --schema <schemaID> --version <versionID>]",
+		Short: "Interact with a collection.",
+		Long:  `Create, read, update, and delete documents within a collection.`,
+		PersistentPreRunE: func(cmd *cobra.Command, args []string) (err error) {


Sweet :) I was hoping you'd be able to find a way to do it like this when I was commenting earlier - forces consistency and we get all the features :)

AndrewSisley · 2023-10-02T13:07:39Z

cli/collection_create.go

+		`,
+		Args: cobra.RangeArgs(0, 1),
+		RunE: func(cmd *cobra.Command, args []string) error {
+			col, ok := cmd.Context().Value(colContextKey).(client.Collection)


suggestion: Similar to store, I'd suggest refactoring out cmd.Context().Value(colContextKey).(client.Collection) into a private func, located next to where it is set (collection.go).

AndrewSisley

LGTM, and looking forward to having it in :) Would suggest a quick manual sanity check of a couple of CLI commands before merge, just in case :)

Good job Keenan :)

shahzadlone

Cheers! Just posted one small nitpick.

We can coordinate merging this as would need to turn off the required checks right before merge.

## 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 30 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

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

enable transaction middleware. make http client WithTxn public

efbc15c

add comment about ignored error in cmd/defradb/main.go

26d8f72

AndrewSisley requested changes Sep 29, 2023

View reviewed changes

AndrewSisley reviewed Sep 29, 2023

View reviewed changes

AndrewSisley requested changes Sep 29, 2023

View reviewed changes

nasdf and others added 8 commits September 29, 2023 13:34

simplify cli store context value retrieval

f0c89b9

fix getall command names

25f36ee

fix license header year

8158750

rename cli document commands to collection

dd34f90

cleanup cli errors

789476f

update cli schema migration up and down documentation

d8b5827

move client wrappers to tests/clients package

3255d05

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

8788f4b

AndrewSisley reviewed Oct 2, 2023

View reviewed changes

AndrewSisley approved these changes Oct 2, 2023

View reviewed changes

nasdf added 6 commits October 2, 2023 09:17

update run-tests-and-upload-coverage workflow triggers

0ad7eb3

add cli collection context helper

0b3514e

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

abf9978

fixes for cli lens migrate up and down

70b2420

add a few more stdin args

1e65462

fix http cors middleware

cd5d0ac

shahzadlone approved these changes Oct 2, 2023

View reviewed changes

nasdf merged commit 4c3df48 into sourcenetwork:develop Oct 2, 2023

AndrewSisley mentioned this pull request Oct 4, 2023

CLI tests failure due to data race #1594

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: CLI client interface #1839

refactor: CLI client interface #1839

nasdf commented Aug 31, 2023 •

edited

Loading

AndrewSisley left a comment

AndrewSisley Sep 29, 2023

AndrewSisley Sep 29, 2023

nasdf Sep 29, 2023

AndrewSisley Sep 29, 2023

AndrewSisley Sep 29, 2023

AndrewSisley Sep 29, 2023

AndrewSisley Sep 29, 2023

AndrewSisley Sep 29, 2023

AndrewSisley Sep 29, 2023

nasdf Sep 30, 2023

AndrewSisley Sep 29, 2023

nasdf Sep 30, 2023

AndrewSisley Sep 29, 2023

AndrewSisley Sep 29, 2023

nasdf Sep 29, 2023

AndrewSisley Sep 29, 2023

nasdf Sep 30, 2023

AndrewSisley Sep 29, 2023

nasdf Sep 30, 2023

AndrewSisley Sep 29, 2023

nasdf Sep 30, 2023

AndrewSisley Sep 29, 2023

nasdf Sep 30, 2023

AndrewSisley Oct 2, 2023 •

edited

Loading

AndrewSisley Sep 29, 2023

nasdf Sep 29, 2023

AndrewSisley Sep 29, 2023

nasdf Sep 30, 2023

AndrewSisley Oct 2, 2023

AndrewSisley Oct 2, 2023 •

edited

Loading

AndrewSisley Oct 2, 2023

AndrewSisley left a comment

shahzadlone left a comment

		@@ -0,0 +1,36 @@
		// Copyright 2022 Democratized Data Foundation

refactor: CLI client interface #1839

refactor: CLI client interface #1839

Conversation

nasdf commented Aug 31, 2023 • edited Loading

Relevant issue(s)

Description

Tasks

How has this been tested?

AndrewSisley 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

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 Oct 2, 2023 • 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

AndrewSisley Oct 2, 2023 • edited Loading

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

nasdf commented Aug 31, 2023 •

edited

Loading

AndrewSisley Oct 2, 2023 •

edited

Loading

AndrewSisley Oct 2, 2023 •

edited

Loading