feat: Add means to fetch schema #2006

AndrewSisley · 2023-10-26T19:54:03Z

Relevant issue(s)

Resolves #1993

Description

Adds means to fetch schema.

Adds a bunch of funcs to the clients to allow fetching of schema(versions) defined in the system. When the set-default stuff got merged, users were no longer guaranteed (at least on init) to be able to view the schema ids they have created, this change allows them to view them when ever they like.

How has this been tested?

Todo:

Manually test the playground with open api stuff

Also adds a bunch of infrastructure that will be also used by GetSchema funcs in the next few commits.

codecov · 2023-10-26T22:05:37Z

Codecov Report

Attention: 84 lines in your changes are missing coverage. Please review.

Comparison is base (d49c860) 74.08% compared to head (efdf13b) 73.96%.

@@             Coverage Diff             @@
##           develop    #2006      +/-   ##
===========================================
- Coverage    74.08%   73.96%   -0.12%     
===========================================
  Files          247      248       +1     
  Lines        24548    24822     +274     
===========================================
+ Hits         18184    18358     +174     
- Misses        5133     5207      +74     
- Partials      1231     1257      +26

Flag	Coverage Δ
all-tests	`73.96% <69.34%> (-0.12%)`	⬇️

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

Files	Coverage Δ
cli/cli.go	`100.00% <100.00%> (ø)`
db/schema.go	`84.93% <100.00%> (+0.57%)`	⬆️
http/openapi.go	`97.14% <ø> (ø)`
cli/schema_describe.go	`83.93% <83.93%> (ø)`
http/handler_store.go	`77.90% <82.61%> (+0.84%)`	⬆️
db/txn_db.go	`51.12% <50.00%> (-0.25%)`	⬇️
http/client.go	`44.24% <58.82%> (+2.76%)`	⬆️
db/description/schema.go	`57.14% <55.10%> (-0.84%)`	⬇️

... and 9 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 d49c860...efdf13b. Read the comment docs.

nasdf

Looks good! One minor fix in the CLI implementation.

nasdf · 2023-10-26T22:12:35Z

cli/schema_describe.go

+				schemas = s
+			}
+
+			if len(schemas) == 1 {


I think this will cause an issue since the output is expected to be an array in some of the cases.

Ah okay, I'll change it. Would you prefer an array all the time, even when version is provided, or should that still return a single item?

I think following the API would be best. So returning a single one for version and array for others.

Change array logic

nasdf · 2023-10-26T22:19:25Z

client/db.go

+
+	// GetAllSchema returns all schema versions that currently exist within
+	// this [Store].
+	GetAllSchema(context.Context) ([]SchemaDescription, error)


These are great additions to the API! It really helps clarify the distinctions between schemas and collections.

hey @nasdf I just wanted to point out that we try to stick to the conventional commenting style https://conventionalcomments.org/

nasdf · 2023-10-26T22:25:19Z

http/handler_store.go

+	schemaDescribe.AddParameter(schemaSchemaRootQueryParam)
+	schemaDescribe.AddParameter(schemaVersionIDQueryParam)
+	schemaDescribe.AddResponse(200, schemaResponse)
+	schemaDescribe.Responses["400"] = errorResponse


Thanks for keeping these in sync!

Thanks for making it simple enough to do so :)

islamaliev · 2023-10-27T05:40:58Z

cli/schema_describe.go

+		Short: "View schema description.",
+		Long: `Introspect schema types.
+
+Example: view all schema


typo: "schemas" or "schema descriptions"

Not a typo, but, after googling, I am incorrect anyway and this needs changing (possibly in a few places) - I actually thought schema was both plural and singular, like sheep and sheep 😅

Schema is not plural

islamaliev · 2023-10-27T05:50:00Z

client/db.go

+
+	// GetAllSchema returns all schema versions that currently exist within
+	// this [Store].
+	GetAllSchema(context.Context) ([]SchemaDescription, error)


suggestion: rename to GetAllSchemas or GetAllSchemaVersions

Cheers, will change.

Rename to GetAllSchemas

islamaliev · 2023-10-27T06:08:07Z

db/description/schema.go

+	txn datastore.Txn,
+) ([]client.SchemaDescription, error) {
+	prefix := core.NewSchemaVersionKey("")
+	q, err := txn.Systemstore().Query(ctx, query.Query{


suggestion: I think you should use datastore.DeserializePrefix[client.SchemaDescription] for this

I only recently noticed that in one of your PRs and then forgot about it, will have a look, but there is a chance I'll want to leave it as is, as I might feel that calling that function hides too much (as the purpose of this file is to show all that stuff in one place).

Checkout datastore.DeserializePrefix

It would be just one click away from visibility though so I'm not sure the duplication would be worth it.

It is not just about that, but extracting this function to another shared function creates an oddity in this file, which itself hinders readability in some cases.

Worse though is that it inhibits change, if this func needs to change slightly (for whatever reason), it is currently very easy to do so - you just change it, however if it is calling datastore.DeserializePrefix, then either you have to risk serious damage by changing that function, or overcome a physcological barrier and un-refactor it and then make the change - that physcological barrier can be non-trivial in some cases and is frequent cause of large bloated shared functions.

I had a look, and I'm still more comfy with the independent implementations. Will be leaving as is.

islamaliev · 2023-10-27T06:12:37Z

tests/clients/cli/wrapper.go

+}
+
+func (w *Wrapper) GetAllSchema(ctx context.Context) ([]client.SchemaDescription, error) {
+	args := []string{"client", "schema", "describe"}


suggestion: 3 of these methods have almost identical bodies. They differ only by args.

It will look cleaner if you extract the common code and pass only args there

I'm not sure that will be cleaner, as these are very simple functions that have no reason to be coupled via a private function, atm it is quite easy to see what they do, and a private func may damage that. I'll have a quick look though.

Consider private func

I had a look, and I prefer the current setup. Leaving as is.

islamaliev

Overall looks good. I just have few suggestions

fredcarle · 2023-10-27T15:29:59Z

cli/schema_describe.go

+				}
+				schemas = s
+
+			case name != "":


suggestion: It would be good to add a check after the switch to check if name and versionID were given that they are actually related like you did with collections.

Yeah I was in two minds if this should behave like the collection stuff, this is different though, as they are not going to be acting on the collection (saving docs and the like). I'll revisit today and see what happens.

~~- [ ] Revisit multi param schema describe stuff~~

Even if not acting on it, it would be weird to return a set of results event if the provided params make no sense.

I'm going to close this thread as it should match whatever we decide in #2006 (comment)

fredcarle · 2023-10-27T15:58:25Z

cli/schema_describe.go

+				schemas = []client.SchemaDescription{schema}
+
+			case root != "":
+				s, err := store.GetSchemaByRoot(cmd.Context(), root)


question: Why not assign directly to schemas when a slice is returned?

Compilation rules as err is not yet defined, the alternative would be to declare err outside the switch.

err outside the switch would be quite fine.

😆 Both are quite fine and imperfect IMO. Declaring err in the broader scope has the drawback of needlessly pulling a local variable with a short lifetime into a broader scope with a longer lifetime.

Are you asking me to change this?

No. The assignment makes no difference in the number of allocation since a slice is passed by reference. I just wanted to know what was your reasoning behind it. Now I know :)

The assignment makes no difference in the number of allocation since a slice is passed by reference

I wasn't talking in terms of performance, a local, short lived variable has a lower readability cost than a broadly scoped long lived one. Especially in cases when there is no real reason for the increased scope/lifetime where authors may be left wondering if there is a reason that it has such a large scope/lifetime.

I know :)

But if there was a significant cost to the allocation, optimizing it would win over narrowing a variable scope. Imagine thousands of schema descriptions being allocated twice. I just mentioned it here to say that it wasn't something to consider and that your reasoning was good 👍

Ah got it :) Thanks for the explanation (and discussion) :)

fredcarle · 2023-10-27T16:10:20Z

http/handler_store.go

+func (s *storeHandler) GetSchema(rw http.ResponseWriter, req *http.Request) {
+	store := req.Context().Value(storeContextKey).(client.Store)
+
+	switch {


suggestion: This switch should follow the same order as the one in the cli. Although we are constructing the request in the client to match exactly one of these cases, there is nothing preventing someone from manually writing a request with multiple parameters.

The client interface does not support that, so this does not support that. Which means the [schema] CLI should not support that. Which means I was wrong to ask Keenan to make the collection describe CLI stuff to support that.

This means that in both cases (CLI and HTTP for both schema and collection) we should make the params mutually exclusive.

Yeah I think so :( Unless we change the client interface to handle this:

schemas, err := db.GetSchemas(Filter{ Name: "Users", Root: "baenfgddhdhshdsdh", VersionID: "baeghdhsdgsgsg", })

Although, I'd see changing the collection stuff as out of scope here, and with the code freeze coming up a rework could me that users end up getting nothing for 8 more weeks.

I think I'm in favour of leaving both Schema and Collection funcs as is in this PR, and then changing them both together in another ticket.

I think Option[Filter] is overkill and would be annoying more than anything. I would say it's overkill for all of this but I mind it less for the fields than for the function parameter.

I think Option[Filter] is overkill and would be annoying more than anything. I would say it's overkill for all of this but I mind it less for the fields than for the function parameter.

Could have:

interface { GetSchemas(...Filter) }

Maybe, but assuming you were happy enough with doing this in another ticket it is not important to design it here.

Totally ok with doing this in a different ticket. In the mean time do you mind just ordering the switch statement the same as the CLI so that they behave the same way?

ordering the switch statement the same as the CLI

Yes of course, sorry I forgot about that.

Change switch statement

Open ticket for Filter stuff

Is now consistent with the CLI

fredcarle

LGTM!

nasdf

Looks all good to me!

islamaliev

LGTM

## Relevant issue(s) Resolves sourcenetwork#1993 ## Description Adds means to fetch schema. Adds a bunch of funcs to the clients to allow fetching of schema(versions) defined in the system. When the set-default stuff got merged, users were no longer guaranteed (at least on init) to be able to view the schema ids they have created, this change allows them to view them when ever they like.

AndrewSisley added feature New feature or request area/schema Related to the schema system action/no-benchmark Skips the action that runs the benchmark. labels Oct 26, 2023

AndrewSisley added this to the DefraDB v0.8 milestone Oct 26, 2023

AndrewSisley requested a review from a team October 26, 2023 19:54

AndrewSisley self-assigned this Oct 26, 2023

AndrewSisley force-pushed the 1993-get-schemas branch 2 times, most recently from 7a3f94c to 6defbb4 Compare October 26, 2023 21:15

AndrewSisley added 4 commits October 26, 2023 17:45

Add GetSchemaByVersionID

c2d837c

Also adds a bunch of infrastructure that will be also used by GetSchema funcs in the next few commits.

Add GetAllSchema

8508736

Add GetSchemaByRoot

8246ee1

Add GetSchemaByName

208267e

AndrewSisley force-pushed the 1993-get-schemas branch from 6defbb4 to 208267e Compare October 26, 2023 21:45

nasdf requested changes Oct 26, 2023

View reviewed changes

islamaliev reviewed Oct 27, 2023

View reviewed changes

fredcarle reviewed Oct 27, 2023

View reviewed changes

AndrewSisley added 3 commits October 27, 2023 13:25

PR FIXUP - Rework cli array stuff

d9dffa2

PR FIXUP - Pluralize schema(s)

28ad92d

PR FIXUP - Reorder switch statement

efdf13b

Is now consistent with the CLI

AndrewSisley requested review from nasdf, islamaliev and fredcarle October 27, 2023 18:10

fredcarle approved these changes Oct 27, 2023

View reviewed changes

nasdf approved these changes Oct 27, 2023

View reviewed changes

islamaliev approved these changes Oct 27, 2023

View reviewed changes

AndrewSisley merged commit 4427a16 into sourcenetwork:develop Oct 27, 2023
29 checks passed

AndrewSisley deleted the 1993-get-schemas branch October 27, 2023 19:08

feat: Add means to fetch schema #2006

feat: Add means to fetch schema #2006

Conversation

AndrewSisley commented Oct 26, 2023 • edited Loading

Relevant issue(s)

Description

How has this been tested?

codecov bot commented Oct 26, 2023 • edited Loading

Codecov Report

nasdf 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

AndrewSisley Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

nasdf Oct 26, 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

islamaliev Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

AndrewSisley Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Oct 27, 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

AndrewSisley Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islamaliev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Oct 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Oct 27, 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

AndrewSisley Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

nasdf left a comment

Choose a reason for hiding this comment

islamaliev left a comment

Choose a reason for hiding this comment

AndrewSisley commented Oct 26, 2023 •

edited

Loading

codecov bot commented Oct 26, 2023 •

edited

Loading

AndrewSisley Oct 27, 2023 •

edited

Loading

nasdf Oct 26, 2023 •

edited

Loading

islamaliev Oct 27, 2023 •

edited

Loading

AndrewSisley Oct 27, 2023 •

edited

Loading

AndrewSisley Oct 27, 2023 •

edited

Loading

AndrewSisley Oct 27, 2023 •

edited

Loading

AndrewSisley Oct 27, 2023 •

edited

Loading

AndrewSisley Oct 27, 2023 •

edited

Loading

AndrewSisley Oct 27, 2023 •

edited

Loading

AndrewSisley Oct 27, 2023 •

edited

Loading

AndrewSisley Oct 27, 2023 •

edited

Loading

AndrewSisley Oct 27, 2023 •

edited

Loading

AndrewSisley Oct 27, 2023 •

edited

Loading