Immutability

[oprypkhantc]

Closes #581

This is still a wip, but I need your feedback to finish it. Don't mind the code style & PHPStan, I'll fix it later.

A couple of notes on the implementation:

• Cloneable is a tiny temporary workaround to allow cloning readonly objects. Once PHP 8.3 is out, it can be refactored to regular clone + $this->prop = $prop combination
• I wanted to avoid creation of ResolverInterface instances on each resolve to avoid mutations, so instead went with passing the object $source parameter
• ResolverInterface::executionSource is the replacement for ResolverInterface::getObject() - it allows getting the object that the field/method is called on. This is used by #[Security('this.someMethod()')] and for prefetching data #[Field(prefetchMethod: 'someMethodOnThis')]
• Getters on QueryFieldDescriptor and InputFieldDescriptor are gone since properties are now readonly

And a lot of questions @oojacoboo:

1. Currently prefetch is tied into graphqlite, such that it's impossible to implement such a feature in userland: there a hard dependency on it in QueryField. What do you think of extracting it into a separate attribute and a parameter middleware? Something like #[Prefetch('methodName', ['arg1'])] + PrefetchParameterMiddleware? That would simplify the QueryField further and allow implementation of similar features in userland, without graphqlite having to implement them. I know you're against adding more attributes, but this seems logical. Existing prefetechMethod and prefetechArgs parameters of #[Field] can be deprecated and removed later on.

2. Although ResolverInterface::executionSource works, I think it's worthwhile thinking on how to improve that. There are multiple problems currently:

   • checks for null in resolvers, because of the __invoke(object|null $source) signature forced by ServiceResolver, which doesn't need a source. All other resolvers do need one and have ugly runtime checks
   • ->setCallable() on QueryFieldDescriptor which duplicates the logic of some of the existing resolvers
   • most importantly, input constructor parameters are not passed through middlewares, partly due to issues with resolvers. This single-handedly stops me from using constructor parameters

   What do you think of refactoring them too in the scope of 7.x? I have a few ideas on mind.

3. QueryFieldDescriptor and InputFieldDescriptor are both bloated with logic of selecting the $originalResolver, being passed 6 properties $callable, $targetMethodOnSource, $targetPropertyOnSource, $magicProperty, $refMethod and $refProperty solely for that purpose. Instead of doing so we can just accept the $originalResolver as a constructor parameter and make respective properties of resolvers public readonly, so that in the rare cases it's needed they can just do $descriptor->originalResolver->methodName

4. I've only added readonly in places where I refactored things, but it's probably worthy adding everywhere, also dropping the getters. Maybe as part of a different PR. Thoughts?

[oojacoboo]

1. Can't we just make prefetchMethod accept a callable as well? What's the benefit of an additional annotation here?
2. These seem like reasonable improvements. Let's do a separate PR on them though.
3. I like it
4. I think locking down the public API is a good idea, or else we could find ourselves dealing with more BC break issues down the road.

[oojacoboo]

Of note, on the documentation, we only need to update the current src. The versioned docs shouldn't be edited since they represent the defined version, which is already locked in functionality. They only need to be updated for mistakes, not changes to the API.

[oojacoboo]

Also, when it comes to defining the public API, I realize there are 2 camps. With the addition of readonly props and typing, we have far greater control over how properties are managed and therefore the accessibility of the property can be used for defining the API, instead of getters/setters.

I realize this may be where we're headed in PHP land, and it may take some time before that's a standard, if ever. The issue I take with this is that is splits your API into methods and properties and it's not always clear which you should be looking for, especially when using something like autocomplete. Sometimes you may need a getter to massage the output from a property, making it private, and other times you may not, relying on that property being public. This leaves the dev wondering and it's a bit chaotic IMO.

In short, I'm not sold on the trend of moving to public properties for defining the API. It's neat, clean, less code - yes. But, it can also be confusing. Further, most of GraphQLite already uses getters/setters, so a move away from this only bifurcates the public API further.

[oprypkhantc]

1. Can't we just make prefetchMethod accept a callable as well? What's the benefit of an additional annotation here?

We can do without a separate annotation, looking for #[Field] in a PrefetchParameterMiddleware and getting the parameters from that if it's a concern. The benefit is full separation of prefetching & the "core", but that's not necessary. Regardless of the attribute used, we can still improve parameter middlewares to allow doing what QueryField currently does exclusively for prefetching, and use the improvement for prefetching itself.

Of note, on the documentation, we only need to update the current src. The versioned docs shouldn't be edited since they represent the defined version, which is already locked in functionality. They only need to be updated for mistakes, not changes to the API.

Yep that is the result of "Replace all ocurrences", I'll fix that.

The issue I take with this is that is splits your API into methods and properties and it's not always clear which you should be looking for, especially when using something like autocomplete

That's exactly why I proposed refactoring $originalResolver. That, in turn, would allow to also refactor $resolver, which makes both QueryFieldDescriptor and InputFieldDescriptor get-less. That's also why I proposed marking all properties readonly and dropping getters - to make it consistent throughout graphqlite.

I can't say I'm entirely sold either. I've worked on other internal packages and I've managed to avoid getters altogether, aside from interfaces of course. I think there are three main use cases for them:

• a design flaw which can be refactored as is the case with QueryFieldDescriptor::getOriginalResolver()
• a lazy-initialized property which is better solved with other means (like wrapping the value in Lazy objects)
• to implement an interface, which at the moment is unavoidable in PHP

I believe that with careful planning, breaking changes can be (mostly) avoided. All of that said, I completely understand your concerns so I can revert the removal of getters for public APIs. If so, do you think this should also be applied to internal APIs or should we drop the getters (as part of a different PR, of course)?

[oojacoboo]

We can do without a separate annotation, looking for #[Field] in a PrefetchParameterMiddleware and getting the parameters from that if it's a concern. The benefit is full separation of prefetching & the "core", but that's not necessary. Regardless of the attribute used, we can still improve parameter middlewares to allow doing what QueryField currently does exclusively for prefetching, and use the improvement for prefetching itself.

It sounds like you're trying to reverse the order of processing for prefetching. Instead of defining how a prefetch is handled for a given field, on that field, you're wanting to add a middleware that will prefetch for all fields based on some logic?

On the APIs, public and internal, I think we should stick with getters for defining the API. Going immutable will obviously eliminate setters. We don't need getters for every property internally either. That can be something that's selectively chosen when defining the API.

[oprypkhantc]

It sounds like you're trying to reverse the order of processing for prefetching. Instead of defining how a prefetch is handled for a given field, on that field, you're wanting to add a middleware that will prefetch for all fields based on some logic?

Not really, the order and all of the behaviour will stay exactly the same. I'm just not satisfied with the implementation of it. Currently QueryField checks whether a prefetch is needed and if so, changes the resolver to return a deferred result. I want to remove any references to prefetch inside of QueryField and extract them into middlewares. For prefetch, I was thinking of a PrefetchParameterMiddleware:

public function mapParameter(ReflectionParameter $parameter, DocBlock $docBlock, Type|null $paramTagType, ParameterAnnotations $parameterAnnotations, ParameterHandlerInterface $parameterMapper): ParameterInterface
    {
        $prefetchAttribute = $parameterAnnotations->getAnnotationByType(Prefetch::class);
        
        if (!$prefetchAttribute) {
            return $parameterMapper->mapParameter($parameter, $docBlock, $paramTagType, $parameterAnnotations);
        }
        
        return new PrefetchDataParameter($prefetchAttribute->method, $prefetchAttribute->args);
    }

and a PrefetchDataParameter:

public function resolve(object|null $source, array $args, mixed $context, ResolveInfo $info, QueryField $field = null): mixed
    {
        $prefetchBuffer = $context->getPrefetchBuffer($field);
        $prefetchBuffer->register($source, $args); // <--- this is what `QueryField` is currently responsible for

        // QueryField also defers the resolver for prefetch-annotated fields, which could be done on parameter level
        return new Deferred(function () {
            if (! $prefetchBuffer->hasResult($args)) {
                $prefetchResult = $this->computePrefetch($source, $args, $context, $info, $field, $prefetchBuffer);

                $prefetchBuffer->storeResult($prefetchResult, $args);
            }

            return $prefetchResult ?? $prefetchBuffer->getResult($args);
        });
    }

That would allow future features that require deferring the parameter resolution possible. And not only in the graphqlite itself, but in userland code too.

On the APIs, public and internal, I think we should stick with getters for defining the API. Going immutable will obviously eliminate setters. We don't need getters for every property internally either. That can be something that's selectively chosen when defining the API.

Got it. I'll revert the getters then on this PR, then submit two more PRs: one for resolvers, the other for readonly throughout graphqlite.

[oprypkhantc]

The problem with not having a #[Prefetch] is it becomes a guessing game for which of the parameters should contain the result of a prefetch, both for the parameter middleware and the developer:

/**
 * @Field(prefetchMethod="prefetchContacts")
 */
public function repeatName($arg1, $arg2, $arg3): string // <-- of the top of your head, 
                                                       // which of the arguments is the prefetch result?

with prefetch that becomes obvious (and again, trivial to detect for a middleware like PrefetchParameterMiddleware):

/**
 * @Field(prefetchMethod="prefetchContacts")
 */
public function repeatName($arg1, #[Prefetch('something')] $arg2, $arg3): string

This problem is avoided in other places of graphqlite by requiring explicit attributes, like #[InjectUser], #[Autowire] etc

[oojacoboo]

@oprypkhantc not opposed to a #[Prefetch] attribute. I do think the separation of concerns here has a good deal of benefit - I like it. In general, I do want to see a limited set of attributes on this lib. I think it's easy to get carried away with adding new functionality through another attribute. However, I think this one is clear with intent and is deserved. I like the design on this. Let's target this on a different PR.

In designing it, I think the Prefetch should accept a callable, in addition to a string argument. This would allow for more flexibility in prefetch planning through another service, as opposed to always calling local methods.

[oprypkhantc]

This is somewhat done. A PR for prefetch & resolvers refactor is coming; for now I've used some workarounds (with comments and TODOs for them).

Aside from SchemaFactory and related things, there's now only one externally mutable place - TypeInterface::setClass. I'll not be touching that for now, and it's unlikely to cause any problems. There are many places where state is mutated privately (for caching purposes for example) - those I'll look through when adding readonly in a separate PR.

[oprypkhantc]

Regarding #[Prefetch], is removing #[Field(prefetchMethod: 'some')] support an OK breaking change or should we first deprecate it?

[oprypkhantc]

Also while working on #[Prefetch] I found an interesting detail: for some bizarre reason, prefetch method is not called statically but rather on the first (random) source instance. This doesn't make sense at all and could lead to problems (like someone accidentally using one of $this-> properties/methods in the prefetch method).

I believe the more logical solution is to call the method statically when only a method name is specified. Not only will this avoid problems for the users, it'll also simplify the code for graphqlite.

Obviously when a full callable is specified we can just resolve the class from the container instead of calling the method statically. Wdyt?

[oojacoboo]

Regarding #[Prefetch], is removing #[Field(prefetchMethod: 'some')] support an OK breaking change or should we first deprecate it?

Are there any issues or limitations in keeping it deprecated? It'd be ideal to deprecate before removal.

[oprypkhantc]

Are there any issues or limitations in keeping it deprecated? It'd be ideal to deprecate before removal.

No I don't think so; I'll add deprecations then. Also is there a point in supporting the annotation version of #[Prefetch]? That wouldn't be difficult either, but I suppose it won't be too long till annotations support is deprecated.

[oojacoboo]

Annotation support is already deprecated. No need to add an annotation.

https://graphqlite.thecodingmachine.io/docs/doctrine-annotations-attributes

[oojacoboo]

@oprypkhantc We still need to get that deprecation added. Is there anything else remaining on this?

[oprypkhantc]

@oojacoboo Are you talking about Prefetch-related deprecations? If so, this is included in a different in a different PR, where I refactored Prefetch: https://github.com/thecodingmachine/graphqlite/pull/588/files#diff-44cccfa8f1939494e34146f50de7ef43e88de7e1789ddc53ec4dd4a813bed6c8R67

[oprypkhantc]

@oojacoboo Hey. Any chance to review this and after this - the prefetch refactor? There are some other issues I'd like to tackle, but first I want these to get merged.

[oojacoboo]

@oprypkhantc this looks great :)

My apologies for the delay in reviewing this. I'll get the others reviewed as well and look forward to any additional PRs.

Before merging this one, can you confirm that we have adequate test coverage for prefetch and the PrefetchBuffer?

[oprypkhantc]

No need to apologize, I know the drill :)

Overall coverage is reported at 96% lines and all of the prefetch seems to be covered, but it's hard to tell for sure because tests don't use #[Covers*] attributes. That said, there's one E2E which covers the feature as a whole, and a couple of unit tests.

Should we reorganize tests a bit? Separate fixtures, unit tests and integration tests and add #[Covers] wherever possible? That'd allow actually using phpunit coverage for coverage reliably.

[oojacoboo]

Should we reorganize tests a bit? Separate fixtures, unit tests and integration tests and add #[Covers] wherever possible? That'd allow actually using phpunit coverage for coverage reliably.

That'd certainly be nice. It'd give us a little more confidence with these merges. Everything else looks good though. I'll go ahead and merge and we can monitor this on master for a bit before tagging a release.

[oprypkhantc]

Thanks! It'd also be awesome to include prefetch and resolvers refactor in that release as well.

[oojacoboo]

Yes - agreed.