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

Support generics #373

Closed
rlch opened this issue Jun 9, 2022 · 8 comments
Closed

Support generics #373

rlch opened this issue Jun 9, 2022 · 8 comments
Labels
enhancement New feature or request

Comments

@rlch
Copy link

rlch commented Jun 9, 2022

Hey there! Thanks for the great driver, works great.

Any plans to support generics? It would be awesome if, for instance, ReadTransaction had something like the following signature:

ReadTransaction[T any](work TransactionWork[T], configurers ...func(*TransactionConfig)) (T, error)

Currently I've built a wrapper around Session to support my use-case, but it would be great to see this implemented within the package.

@fbiville
Copy link
Contributor

fbiville commented Jun 22, 2022

Hello @rich, apologies for the delay in answering, I missed the notification, it seems.

The driver will be fully able to embrace generics, when generic functions on interfaces are supported.
This is not yet the case in 1.18.
So cases like ReadTransaction (also known as ExecuteRead) are stuck with a pre-generics signature, since it lives on the Session interface.

Until then, the best the 5.0 driver can offer is here: https://github.com/neo4j/neo4j-go-driver/blob/5.0/neo4j/result_helpers.go. This exposes a few standalone generic functions to extract results.

I'm of course open to ideas for improvements.

@fbiville
Copy link
Contributor

fbiville commented Oct 17, 2022

To add to my initial answer, let me elaborate on why the Go driver cannot fully embrace generics (let's take Session.Run* as an example).

A generic version of Session that would work today (Go 1.19) would be like this:

type Session[T any] interface {
    // [...]
    Run(ctx context.Context, cypher string, params map[string]any, configurers ...func(*TransactionConfig)) (Result[T], error)
}

// assuming SessionConfig would include:

type SessionConfig[T any] interface {
    // [...]
    ResultMapper func(Result) T
}

The biggest issue I have with this is that a session instance is tied to a single type constraint definition.

Users wouldn't be able to call Run for a query that returns a string and another one that returns a int64 unless they define a type constraint of string | int64.

You can see how this does not scale well with the number of calls one has to do on Session.

Moreover, a type assertion would still be necessary to get the actual type required for each particular call, so the type assertion ceremony would still be needed.

The situation is actually worse than that, since the type constraint need to be propagated back to the session creation API, which is defined at the driver level.

As we've seen before, type constraints in interfaces cannot be declared at the function level, they need to be at the type level, so we would end up with:

type Driver[T any] interface {
    // [...]
    NewSession(config SessionConfig[T]) Session[T]
}

It is strongly advised that Driver is instantiated once for the whole application lifetime. This would mean that T needs to be a union of all possible result types.

That's just an appalling developer experience right there.

*the actual names are DriverWithContext, SessionWithContext ResultWithContext but kept shorter for clarity

@rlch
Copy link
Author

rlch commented Oct 18, 2022

Yep, that makes a lot of sense. What if we kept Driver type-argument-less and created a new type:

type TypedDriver[T any] interface {
  Driver
  ...
  NewSession(config SessionConfig[T]) Session[T]
}

or we made func NewSession[T] a top-level function which takes an instance of Driver? We could even go a layer deeper so that Session doesn't need to have type-arguments as well by instead making Run, WriteTransaction etc top-level taking a Session.

Type arguments on methods doesn't seem like it's coming anytime soon: https://go.googlesource.com/proposal/+/refs/heads/master/design/43651-type-parameters.md#No-parameterized-methods and I feel like this is a decent solution for the meantime.

@fbiville
Copy link
Contributor

fbiville commented Oct 18, 2022

I like the idea of extracting standalone functions. In that case, the candidates would likely be:

  • Session.Run -> Run[T any](Session, ...) (Result[T], error) (not worth it, since Result can be mapped by the user directly)
  • Session.Execute{Read;Write} -> Execute{Read;Write}[T any](Session, ...) (T, error)

I'll discuss this internally and report back.

@rlch
Copy link
Author

rlch commented Nov 23, 2022

Hey,

Just wanted to provide these helper functions we've been using in place of this feature for anyone interested:

package helpers

import (
	"encoding/json"
	"fmt"
	"reflect"

	"github.com/neo4j/neo4j-go-driver/v4/neo4j"
)

// UnmarshalProps unmarshals the props into an instance of T. If T is an interface, it will choose the appropriate implementation
// based off the first label in labels that matches the Neo4J label of the implementer.
func UnmarshalProps[T any](props map[string]interface{}, labels []string, implementers ...T) (*T, error) {
	bytes, err := json.Marshal(props)
	if err != nil {
		return nil, err
	}

	var out T
	rType := reflect.TypeOf((*T)(nil)).Elem()
	if rType.Kind() == reflect.Interface {
		for _, label := range labels {
			for _, st := range implementers {
				tag := ExtractNeo4JLabel(st)
				if tag == label {
					stType := reflect.TypeOf(st)
					out = reflect.New(stType).Interface().(T)
					if err := json.Unmarshal(bytes, &out); err != nil {
						return nil, err
					}
					return &out, nil
				}
			}
		}
		return nil, fmt.Errorf("the node or relationship has no label that matches any of the provided subtypes")
	} else {
		if err := json.Unmarshal(bytes, &out); err != nil {
			return nil, err
		}
		return &out, nil
	}

}

// UnmarshalRecord unmarshals the node, relationship or T corresponding to the provided key into T.
func UnmarshalRecord[T any](record *neo4j.Record, key string, implementers ...T) (*T, error) {
	result, ok := record.Get(key)
	if !ok {
		return nil, fmt.Errorf("the key: %s does not exist in the provided record. Expected one of %v", key, record.Keys)
	}
	if out, ok := result.(T); ok {
		return &out, nil
	}
	var props map[string]interface{}
	var labels []string
	node, isNode := result.(neo4j.Node)
	if isNode {
		props = node.Props
		labels = node.Labels
	}
	edge, isEdge := result.(neo4j.Relationship)
	if isEdge {
		props = edge.Props
		labels = []string{edge.Type}
	}
	rawJSON, isJSON := result.(map[string]interface{})
	if isJSON {
		props = rawJSON
	}
	if !isNode && !isEdge && !isJSON {
		return nil, fmt.Errorf("the value is of type %T, expected neo4j.Node, neo4j.Relationship or map[string]interface{} of %T", result, new(T))
	}
	return UnmarshalProps(props, labels, implementers...)
}

// UnmarshalSingleNode gets the single record from the provided result, and unmarshals the node,
// relationship or T corresponding to the provided key into T.
func UnmarshalSingleRecord[T any](result neo4j.Result, key string, implementers ...T) (*T, error) {
	record, err := result.Single()
	if err != nil {
		return nil, err
	}
	return UnmarshalRecord(record, key, implementers...)
}

func UnmarshalRecords[T any](result neo4j.Result, key string, implementers ...T) ([]T, error) {
	records, err := UnmarshalPtrRecords(result, key, implementers...)
	if err != nil {
		return nil, err
	}
	out := make([]T, len(records))
	for i, v := range records {
		out[i] = *v
	}
	return out, nil
}

func UnmarshalPtrRecords[T any](result neo4j.Result, key string, implementers ...T) ([]*T, error) {
	var out []*T
	for result.Next() {
		record := result.Record()
		item, err := UnmarshalRecord(record, key, implementers...)
		if err != nil {
			return nil, err
		}
		out = append(out, item)
	}
	return out, nil
}

@fbiville
Copy link
Contributor

Hello @rlch, would #415 work for you?

fbiville added a commit that referenced this issue Dec 12, 2022
fbiville added a commit that referenced this issue Dec 13, 2022
fbiville added a commit that referenced this issue Dec 13, 2022
These are generic helpers to extract properties from nodes and
relationships in a type-safe way.

Relates to #373
@fbiville
Copy link
Contributor

I also opened #416 since that seems to be a quick win.

fbiville added a commit that referenced this issue Dec 15, 2022
fbiville added a commit that referenced this issue Dec 15, 2022
These are generic helpers to extract properties from nodes and
relationships in a type-safe way.

Relates to #373
fbiville added a commit that referenced this issue Dec 15, 2022
fbiville added a commit that referenced this issue Dec 15, 2022
These are generic helpers to extract properties from nodes and
relationships in a type-safe way.

Relates to #373
@fbiville fbiville added the enhancement New feature or request label Dec 21, 2022
@fbiville fbiville changed the title [Feature Request] Support generics Support generics Dec 21, 2022
@fbiville
Copy link
Contributor

Since this an evergoing quest, I turned the issue into a discussion, which is better suited for this feature.
Please join #444!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants