[WIP] Resolver and Serialisation #21

iDevelopThings · 2022-09-25T17:19:18Z

The idea/solution i mentioned in #20

Implemented RPCRawResponse/Serialisation changes
Implemented the query resolver solution
Started tests for resolver/serialisation

Only handled the regular Query() function so far
It's pretty hacky in some places, was a quick put together as POC

TODO: Need to clean up the panics and find better solutions to them

…e and provides ability to define scope, ns, db etc

This will allow us to do individual tests without re-creating db each time Wrote tests for `SignupUser` & `SigninUser`

- Implemented RPCRawResponse/Serialisation changes - Implemented the query resolver solution - Started tests for resolver/serialisation TODO: Need to clean up the panics and find better solutions to them

env_util.go

Keitio · 2022-09-25T18:04:37Z

err.go

-func (self PermissionError) Error() string {
-	return fmt.Sprint("Unable to access record:", self.what)
+func (pe PermissionError) Error() string {
+	return fmt.Sprint("Unable to access record:", pe.what)


Instead of Sprint, you can just return "Unable to access record: " + pe.what

Github Copilot 😅 will change

Keitio · 2022-09-25T18:05:50Z

query_resolver.go

+
+// Query creates a new query resolver
+// It would be ideal if we create a global for *DB
+func Query[T any](db *DB, ctx context.Context, query string, params ...any) *QueryResolver[T] {


The context should always be the first parameter here
And creating a global db would be kind of nice to use, but horrendous to test and using multiple db connections would be near impossible

Keitio · 2022-09-25T18:06:58Z

query_resolver.go

+func Query[T any](db *DB, ctx context.Context, query string, params ...any) *QueryResolver[T] {
+	var paramsData = QueryParams{}
+	if len(params) > 0 {
+		paramsData = params[0].(QueryParams)


This is extremely unsafe to do, just take a params ...QueryParams instead, and document that only the first one is used

Keitio · 2022-09-25T18:07:26Z

query_resolver.go

+	Time   string `json:"time"`
+}
+
+type QueryParams = map[string]any


Maybe this should only be called Map ? It would be easier to use

Keitio · 2022-09-25T18:10:26Z

query_resolver.go

+	return resolver.resolve(db)
+}
+
+func (r *QueryResolver[T]) resolve(db *DB) *QueryResolver[T] {


The first line of this method should probably be something along the lines of

if r.err != nil { return r }

so we don't work on an invalid object

This r.err is the error we found during processing of the RPC Response, not a local error with the QueryResolver ^^

Keitio · 2022-09-25T18:12:35Z

rpc.go

+
+	decodedError bool
+	hasError     bool
+	error        *RPCError


those 3 members look strange to me, hasError should be error != nil IMO
and document what decodedError is

These are mainly just states so we know that we've processed looking for the error in the raw bytes,
decodedError = did we try to decode it?
hasError = we decoded it & we found an error

Keitio · 2022-09-25T18:13:43Z

rpc.go

+	return nil
+}
+
+func (res *RPCRawResponse) HasError() bool {


This is wayyy too complicated for a Go error check
The way to check errors in go is if err != nil, so this work should be done before the user gets the result back

Agreed, it needs some refactoring

My Idea was that we run the check once, and then we rely on the error/hasError states from the above suggestion

Keitio · 2022-09-25T18:14:49Z

types.go

+type TokenData struct {
+	IssuedAt  int    `json:"iat"`
+	NotBefore int    `json:"nbf"`
+	ExpiresAt int    `json:"exp"`


Those should be time.Time (this implies a bit more work on the driver side)

Keitio · 2022-09-25T18:17:24Z

types_test.go

+	"testing"
+
+	"github.com/surrealdb/surrealdb.go"
+	"github.com/test-go/testify/suite"


If we could not use testify that would be great, Go has a fantastic standard library that does most of the work for us, and testify is a huge dependency (go has no concept of dev dependencies, so the entire testify and its many dependencies are pulled if you want to use this driver)

iDevelopThings · 2022-09-25T18:59:57Z

Sorry for any spam 😅

I will see if i can work through these changes tomorrow and clear some bits up + add the go build tags for 18+

iDevelopThings · 2022-09-26T13:47:33Z

@Keitio what would we do with this panic here?

We wouldn't want to cancel the context/close the ws right? 🤔

Keitio · 2022-09-26T13:57:32Z

Yeah, logging the error and then a continue would be more appropriate

- Complete refactor of a lot of the logic and some further abstractions - Added handling for the regular crud methods, create, update, modify etc - Commented a lot of the code I wrote Issues: - Currently there's a problem where the normal db.create, update etc, fail because the db send method returns RPCRawResponse, not sure how to handle that situation at the moment

iDevelopThings · 2022-09-26T17:29:31Z

F, I thought ready for review, would request another one, my bad...

Keitio

My opinion is that i find the API too large to be searchable, and that's for a simple CRUD (mostly) driver
I personally think we should strive for the simplest API that's useful because it will be better understood (thus less isssues)
I think this is mostly due to exposing internal implementation details way too much (IMO even the WS should be hidden to end users)
This also doesn't strike me as very "go-like", but that's probably a me issue

Keitio · 2022-09-26T18:48:11Z

query_resolver.go

+	"context"
+)
+
+type QueryResolver[T any] struct {


Should this type be exported ?

Keitio · 2022-09-26T18:49:21Z

query_resolver_handlers.go

+
+var (
+	// ErrResolvedQueryResultIsInvalid Need a better name :|
+	ErrResolvedQueryResultIsInvalid = errors.New("the result from the database response is not valid, expected an array")


ErrInvalidRpcResult ? make it so that it covers more cases than just array expected, but just a general rpc result problem

iDevelopThings · 2022-09-26T22:01:31Z

Yeah I agree, it's quite large, but it's more of an optional/additional layer the dev can opt into to keep things a bit more simplified, for example, using .Query() and getting your individual item out of the result each time, gets boring fast, and then you write a wrapper to do it.... then every person does the same thing, it makes sense to have an option for that being handled

Things that you think shouldn't be exposed like that, can be sorted, it's just a WIP, on my personal code, i don't usually care too much about things not being exported 😅

Keitio · 2022-09-26T22:43:50Z

Yep, no problems with that ! Was just stating my personal opinion on the current state of your branch just for you to get some (albeit very opinionated) feedback

phughk · 2023-05-23T14:21:33Z

Hey @iDevelopThings 👋 Thanks for opening this! Do you want to continue trying to merge this or can we close this PR? Will assign @timpratim if we do decide to continue. It would be good to resolve the merge conflicts to make it easier to review.

Keitio and others added 5 commits September 23, 2022 20:25

Added context to most methods needing one, various code cleanups

084c35c

Extended UserInfo & Token Data struct + tests

9507ad9

SigninUser & SignupUser method which returns a more typed respons…

395f66b

…e and provides ability to define scope, ns, db etc

Test Suite setup in db_test(left old tests alone)

76e52b0

This will allow us to do individual tests without re-creating db each time Wrote tests for `SignupUser` & `SigninUser`

[WIP]

e663267

- Implemented RPCRawResponse/Serialisation changes - Implemented the query resolver solution - Started tests for resolver/serialisation TODO: Need to clean up the panics and find better solutions to them

Keitio suggested changes Sep 25, 2022

View reviewed changes

iDevelopThings marked this pull request as ready for review September 26, 2022 17:29

Keitio reviewed Sep 26, 2022

View reviewed changes

tobiemh marked this pull request as draft September 27, 2022 23:38

phughk added the stale label May 23, 2023

phughk assigned timpratim May 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Resolver and Serialisation #21

[WIP] Resolver and Serialisation #21

iDevelopThings commented Sep 25, 2022 •

edited

Loading

Keitio Sep 25, 2022

iDevelopThings Sep 25, 2022

Keitio Sep 25, 2022

Keitio Sep 25, 2022

Keitio Sep 25, 2022

Keitio Sep 25, 2022

iDevelopThings Sep 26, 2022

Keitio Sep 25, 2022

iDevelopThings Sep 25, 2022

Keitio Sep 25, 2022

iDevelopThings Sep 25, 2022

Keitio Sep 25, 2022

Keitio Sep 25, 2022

iDevelopThings commented Sep 25, 2022

iDevelopThings commented Sep 26, 2022

Keitio commented Sep 26, 2022

iDevelopThings commented Sep 26, 2022

Keitio left a comment

Keitio Sep 26, 2022

Keitio Sep 26, 2022

iDevelopThings commented Sep 26, 2022

Keitio commented Sep 26, 2022

phughk commented May 23, 2023

[WIP] Resolver and Serialisation #21

Are you sure you want to change the base?

[WIP] Resolver and Serialisation #21

Conversation

iDevelopThings commented Sep 25, 2022 • 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

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

iDevelopThings commented Sep 25, 2022

iDevelopThings commented Sep 26, 2022

Keitio commented Sep 26, 2022

iDevelopThings commented Sep 26, 2022

Keitio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iDevelopThings commented Sep 26, 2022

Keitio commented Sep 26, 2022

phughk commented May 23, 2023

iDevelopThings commented Sep 25, 2022 •

edited

Loading