Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle slice input with SmartUnmarshal #78

Open
rytswd opened this issue Jul 17, 2023 · 3 comments
Open

Handle slice input with SmartUnmarshal #78

rytswd opened this issue Jul 17, 2023 · 3 comments
Labels
awaiting-feedback We need more information before continuing enhancement New feature or request

Comments

@rytswd
Copy link

rytswd commented Jul 17, 2023

While SmartUnmarshal is useful with its generics support, it needs an explicit handling for handling a slice input. As we can check further into the unmarshalled data in RawQuery, it would be great to add some handling for complex data structure. This comes with a bit of cost with potential recurses, and thus it is probably preferable to keep the existing logic, and add a new function instead.

@phughk phughk added enhancement New feature or request awaiting-feedback We need more information before continuing labels Jul 21, 2023
@phughk
Copy link
Contributor

phughk commented Jul 21, 2023

Hey @rytswd , could you perhaps provide an example of how you think this could work? Do you have example data structures? Is this about ordering of keys? Does slice work for Unmarshal instead of SmartUnmarshal?

@rytswd
Copy link
Author

rytswd commented Jul 21, 2023

Hi @phughk! Sure, for the data structure, do you mean what return type we should use? In my code in the PR, I simply defined a new func SmartUnmarshalAll[T] to return []T instead of T. (I also added a note how one could call with slice type such as SmartUnmarshal[[]User]( ... ), but this looks quite foreign to me...)

For the example, I actually shared my finding in a comment in the above PR:

	data, err = db.Query(`
		CREATE user:j SET name = "J";
		CREATE user:j SET name = "J"; // conflict
		CREATE user SET name = "John";
		`, nil)
	if err != nil {
		t.Fatal(err) // Not hit
	}
	y, err := surrealdb.SmartUnmarshal[[]User](data, nil)
	t.Logf("old data: %+v", y)   // old data: [{Username: Password: ID:user:j Name:J}]
	t.Logf("old err:  %+v", err) // old err:  <nil>

	x, err := surrealdb.SmartUnmarshalAll[User](data)
	t.Logf("new data: %+v", x)   // new data: [{Username: Password: ID:user:j Name:J} {Username: Password: ID:user:3owq0tv8kn0aiz1rc6od Name:John}]
	t.Logf("new err:  %+v", err) // new err: ERR: Database record `user:j` already exists

Because the RawQuery can be nested, the generic handling needs to handle RawQuery as any type, and recursively unmarshal. This could potentially cause degraded performance, and Unmarshal may be a better fit. But as there is already SmartUnmarshal in place, I'd think it should at least treat the errors and data write path fully.

It is arguable what the idiomatic Go implementation should look like with generics, but having to specify []User rather than User for the type input seems unnatural to me.

This is potentially an area where godoc clarification may be sufficient, as long as the SmartUnmarshal can handle bulk input with full details of all items written and any errors hit.

Hope this makes sense, and if there is any more clarifications needed, I'd be happy to share!

@phughk
Copy link
Contributor

phughk commented Dec 1, 2023

It's definitely a good idea, but I am wondering if we would rather use Row and Column from the upcoming changes? It might result in a simpler solution to use, and reduce the number of options for deserialising

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-feedback We need more information before continuing enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants