Refactor needed in typesystem resolver

[miparnisari]

We have a minor design concern in our code base.

We should never use server.typesystemResolver directly within the server handlers, we should always use server.resolveTypesystem because that is the one that returns actual server errors (with correct http status codes).

I.e. with typesystemResolver

curl -v --location 'localhost:8080/stores/01HTZAKTWJ5Y69NVC5TMRW7RGY/list-users' \
--data '{
    "authorization_model_id": "01HV4DX6NX1WNTFVDVAQW4WR58", // not found
    "object": {"type": "folder", "id": "5"},
    "relation": "can_read",
    "user_filters": [{"type": "user"}]
}'

< HTTP/1.1 500 Internal Server Error
{"code":"internal_error","message":"authorization model not found"}

versus with resolveTypesystem

curl -v --location 'localhost:8080/stores/01HTZAKTWJ5Y69NVC5TMRW7RGY/list-objects' \
--header 'Content-Type: application/json' \
--header 'Accept: application/json' \
--data '{
    "authorization_model_id": "01HV4DX6NX1WNTFVDVAQW4WR58", // not found
    "type": "document",
    "relation": "viewer",
    "user": "user:jon"
}'

< HTTP/1.1 400 Bad Request
{"code":"authorization_model_not_found","message":"Authorization Model '01HV4DX6NX1WNTFVDVAQW4WR58' not found"}

The way the code is structured today makes it very easy to make this mistake, as seen in #1516 (comment).

IMO, the typesystem resolver code is better written as a command (e.g. https://github.com/openfga/openfga/pull/1258/files#diff-679325381eda2c29f2d4794339a1efa278a4f65cdebc7514b9f45341c5f59ec8)

[jon-whit]

Here are some thoughts I wanted to share:

1. The Typesystem package should not know anything about the Server layer. Server specific error handling and error codes are not the responsibility of the Typesystem. So let's maintain that single responsibility principle.
2. The Server only needs a single Typesystem resolver. We shouldn't be creating a new resolver on every request, as doing so would be very wasteful. A single instance of anything that implements the TypesystemResolverFunc signature should be sufficient for the whole server to share across any RPC.
3. The Typesystem resolver that the Server uses should be derived off of the MemoizedTypesystemResolverFunc in the typesystem package, because that provides a common implementation that provides caching/memoization. Implementing that in a second way would be duplication of code and responsibility.
4. A command shouldn't be any piece of code needed somewhere. That is what packages are for. Commands in our code have been reserved for RPC Server handler implementations. Creating a Command for typesystem resolution would be overreaching responbility given our current usage patterns and our current established standards.

Given the points above, my suggestion is simple. We drop the s.resolveTypesystem function, and when constructing a Server, initialize s.typesystemResolver with a wrapped closure over the typesystem.MemoizedTypesystemResolverFunc. For example,

// pkg/server/server.go
package server

func NewServerWithOpts(...) {
    s := &Server{...}
    ...
    s.typesystemResolver, s.typesystemResolverStop = s.serverTypesystemResolver()
}

...

// serverTypesystemResolver constructs a [[typesystem.TypesystemResolverFunc]] that
// resolves the underlying TypeSystem given the storeID and modelID and
// it sets some Server response metadata based on the model resolution.
func (s *Server) serverTypesystemResolver() (typesystem.TypesystemResolverFunc, func()) {
	memoizedResolver, typesystemResolverStop := typesystem.MemoizedTypesystemResolverFunc(s.datastore)

	return func(ctx context.Context, storeID, modelID string) (*typesystem.TypeSystem, error) {
		ctx, span := tracer.Start(ctx, "resolveTypesystem")
		defer span.End()

		typesys, err := memoizedResolver(ctx, storeID, modelID)
		if err != nil {
			if errors.Is(err, typesystem.ErrModelNotFound) {
				if modelID == "" {
					return nil, serverErrors.LatestAuthorizationModelNotFound(storeID)
				}

				return nil, serverErrors.AuthorizationModelNotFound(modelID)
			}

			if errors.Is(err, typesystem.ErrInvalidModel) {
				return nil, serverErrors.ValidationError(err)
			}

			return nil, serverErrors.HandleError("", err)
		}

		resolvedModelID := typesys.GetAuthorizationModelID()

		span.SetAttributes(attribute.KeyValue{Key: authorizationModelIDKey, Value: attribute.StringValue(resolvedModelID)})
		grpc_ctxtags.Extract(ctx).Set(authorizationModelIDKey, resolvedModelID)
		s.transport.SetHeader(ctx, AuthorizationModelIDHeader, resolvedModelID)

		return typesys, nil
	}, typesystemResolverStop
}

This way everywhere in our code in the server when we use s.typesystemResolver(ctx, storeID, modelID) we'll inherit the Server specific error handling, and we will still have a clear seperation of responsibility, and we'll avoid introducing a command superfluously.

[miparnisari]

@openfga/backend any thoughts on this? Would appreciate more inputs :)

---

@jon-whit Your suggestion is a good N+1 because it solves the problem I mentioned. The suggestion I made was for N+2 😄

A command shouldn't be any piece of code needed somewhere. That is what packages are for.

I disagree with this. Packages are meant to put related code close together. The reusability/composabilty aspect is achieved via other means, including but not limited to design patterns such as command.

Commands in our code have been reserved for RPC Server handler implementations. Creating a Command for typesystem resolution would be overreaching responbility given our current usage patterns and our current established standards.

This is true, but my whole point is that there is nothing preventing us from changing this pattern of "1 api <-> 1 command" to "1 api -> 1 command" :)

Creating a Command for typesystem resolution would be overreaching responbility given our current usage patterns and our current established standards.

We've never explicitly defined what our standards were in this regard. And yes, what I am suggesting is a deviation from all the other commands, in that we wouldn't expose it as an API. But! it would be a very trivial change to do so if we wanted (#1382)

This way everywhere in our code in the server when we use s.typesystemResolver(ctx, storeID, modelID) we'll inherit the Server specific error handling,

We don't need this split - the only caller of s.typesystemResolver is s.resolveTypesystem, so why put the code apart if it is needed as a unit.

and we will still have a clear seperation of responsibility, and we'll avoid introducing a command superfluously.

• Today, commands mostly return http status codes directly, why should the typesystem resolver be different?
• If introducing a command allow us to remove the duplicate cache of models, the superfluousness would be cancelled out many times over.

[jon-whit]

I disagree with this. Packages are meant to put related code close together. The reusability/composabilty aspect is achieved via other means, including but not limited to design patterns such as command.

The solution I proposed puts related code closer together in the same defining package. I see no difference. The question is - does this usage pattern/need warrant needing a completely separate abstraction known as a command?

You don't need to construct a command every time you need to resolve a model, you just need a function that can be shared across code modules that can resolve the typesystem. That extends well beyond the scope of needing to create a new object/struct in memory every single time you need to resolve a typesystem. You wouldn't create a Server every time you needed to resolve an RPC, so why would we create a TypesystemResovlerCommand every time we need to resolve a typesystem?

Furthermore, if everything that implements business logic is a command, then at what point does the term command just become a proxy in our code for "function"? I don't understand the intent there.

This is true, but my whole point is that there is nothing preventing us from changing this pattern of "1 api <-> 1 command" to "1 api -> 1 command" :)

I understand where you are coming from. You want to achieve more consistency in the code base, and I think that's a good goal, but the way of achieving that with commands is not a good approach and is wasteful. Creation of command objects is superfluous for code sharing in our project since it is not shared across any other components besides the RPC handlers. The commands are effectively the RPC handlers. If we introduced v2 RPC handlers we'd gain no value of our existing commands. We only use them in our RPC handlers so we're just creating new objects/structs on every RPC invocation for little to no gain. I go back to my example above - you wouldn't create a Server every time you needed to resolve an RPC, so why would we create a command every time we need to resolve an RPC handler?

What we should be prioritizing here is consistent code re-use through shared components. If we're not sharing components then creation of a new object on every request for no shared value is wasteful. Sharing code that resolves a typesystem is good, but that doesn't warrant an independent structure per request to do so.

Today, commands mostly return http status codes directly, why should the typesystem resolver be different?

Because it's not the typesystem's responsibility to understand the Server transport layer. That would leak transport level details in the wrong direction. We wouldn't return HTTP status codes to communicate errors from the storage layer. Why would the typesytem be treated any differently? The typesystem resolves types - it has nothing to do with the Server level transport.

If introducing a command allow us to remove the duplicate cache of models, the superfluousness would be cancelled out many times over.

The shared TypesystemResolver pattern described in my previous comment achieves the goal of removing duplicate cache of models as well. Why is a command needed over a shared function? What value does introducing another struct into the mix have over a shared function?