HSEARCH-5235 WIP: draft of injecting field references #4451
Conversation
| @@ -154,6 +157,8 @@ public void configure(MappingBuildContext buildContext, PojoMappingConfiguration | |||
| new PojoAnnotationTypeMetadataDiscoverer( contributorFactory, alreadyContributedTypes ); | |||
| collector.collectDiscoverer( discoverer ); | |||
| } | |||
|
|
|||
| injectableBinderCollector.processDiscoveredBinders(); | |||
There was a problem hiding this comment.
I've tried a few things and here's where I ended up so far 😃
My thinking is that since we want these new binders as a way to have annotated properties so we can read that info in an AP, it seems that making them "available" through the programmatic mapping probably doesn't make that much sense...
Hence, I thought that we could "detect" used binders when an annotation processor (current Search ones to get info from annotations) use a corresponding binder method ( which matches the case when a user adds a binder. So it'll include those coming from programmatic API too ... but IDK if we should just prohibit that use from the programmatic mapping or simply "fail nicely"). Then, once we "discovered" all annotations, we process the info we collected on binders by creating "something" that later will be able to inject required fields into a binder instance.
Since binder metadata (fields to inject) will not change depending on where the binder is used, we can have an instance per type rather than per occurrence.
Then, the injection of fields happens here right before the bind method is called. We do have that delegating binder, which creates a bit of a problem since we cannot simply get our hands on the actual binder instance. I've worked around that for now by updating the delegating binder.
I've started by trying to reuse our annotation processors and basically building an "index model" for a binder but discarded that idea as it didn't work out that nicely 😄
Thinking more about it, for an annotation processor ... we won't be able to rely on user custom annotations and their processors anyway... so in this patch, I've just put together some simple processing to get the required data to create a field reference just for this injection part... thinking even more about it since we will be limiting the number of processors to a specific subset maaaybe I could make it work (reuse current processors at least partially).
Speaking of custom annotations... that's something that also won't play nice with the static metamodel, I suppose, unless we also do some similar "injection of field references" there, too ....
Does that sound to you like the direction to keep exploring @yrodiere ? or should I revisit the approach 😃 🙈
There was a problem hiding this comment.
Sorry I won't be able to review this before I'm back in January, but here are a few answers...
My thinking is that since we want these new binders as a way to have annotated properties so we can read that info in an AP, it seems that making them "available" through the programmatic mapping probably doesn't make that much sense...
I think it could make sense in some cases. E.g. if parts of your mapping are static (so you'll be able to use a static metamodel) and others are not, and if you need similar binders in both cases: it'd be nice to be able to use the same binder in both places.
Even if that was not true, I think we probably want (eventually) to have a single way to implement binders, just to reduce the maintenance burden on our side...
Hence, I thought that we could "detect" used binders when an annotation processor (current Search ones to get info from annotations) use a corresponding binder method ( which matches the case when a user adds a binder.
I don't understand this, I need to have a look at the code.
Since binder metadata (fields to inject) will not change depending on where the binder is used, we can have an instance per type rather than per occurrence.
I think.we talked about some dynamism involving an expression language, which might change a few things such as analyzers based on where the binder is used ?
We do have that delegating binder, which creates a bit of a problem since we cannot simply get our hands on the actual binder instance. I've worked around that for now by updating the delegating binder.
We could reasonably add a new limitation that binders -- at least those using injected fields -- cannot be CDI beans. That would probably remove the need for delegating binders. Maybe.
Speaking of custom annotations... that's something that also won't play nice with the static metamodel, I suppose, unless we also do some similar "injection of field references" there, too ....
I think the current way to implement custom annotations won't work. But yes we can find a way -- later.
There was a problem hiding this comment.
Oh yeah sure 😃 I only wanted to put those few notes so I don't forget about them 🙈 let's definitely leave this discussion for the new year 😃
I think.we talked about some dynamism involving an expression language, which might change a few things such as analyzers based on where the binder is used ?
Right.. we did. I think in that case the dynamic part will come from the parameters, which we will have at the binding time, so my plan so far, was to pass those to the "injector" (at binding time) and it will do the work. But yes - let's get back to that next year 😃
There was a problem hiding this comment.
And thank you for the answers 😃
marko-bekhta
force-pushed
the
feat/HSEARCH-5235-Custom-annotations-and-binders-compatible-with-javacs-annotation-processor
branch
from
4e062a6 to
a6d1f74
Compare
| context.dependencies() | ||
| .useRootOnly(); |
There was a problem hiding this comment.
Interestingly, if we found a way to declare dependencies through annotations too... we could get rid of the binder altogether, and declare everything directly in the bridge.
In fact if we do that, I wonder if there's even any reason to keep binders when using annotations to configure the binding? They might only be useful for the old, very-dynamic API, which we'll probably want to deprecate anyway.
| * | ||
| * @param <T> The value type. | ||
| */ | ||
| public interface ValueReadWriteHandle<T> extends ValueReadHandle<T>, ValueWriteHandle { |
There was a problem hiding this comment.
I'm struggling to understand why this is necessary... in most cases I'd expect callers to need a read handle, or a write handle, but not both. Even when both are needed, there's nothing preventing you from having two instances: the read handle, and the write handle.
Wouldn't it simplify many things to just keep the two interfaces separate?
I imagine you would be able to remove generics here and there, in particular in the PojoRawTypeModel and similar... don't override the return type of handle(), just expose two methods writeHandle() and readHandle()?
| /** | ||
| * @return A handle to read the value of this property on a instance of its hosting type. | ||
| */ | ||
| ValueReadWriteHandle<T> handle(); |
There was a problem hiding this comment.
FWIW it seems simpler to me to just expose this directly on PojoPropertyModel.
Yes this might involve runtime errors, but there's a limit to how type safe you can be, while keeping things understandable...
| import org.hibernate.search.util.common.annotation.Incubating; | ||
|
|
||
| @Incubating | ||
| public interface PojoInjectableBinderModel<T> extends PojoRawTypeModel<T> { |
There was a problem hiding this comment.
From what I can see, if you get rid of PojoInjectablePropertyModel as I suggested, this can disappear as well.
| import org.hibernate.search.mapper.pojo.model.spi.PojoInjectablePropertyModel; | ||
| import org.hibernate.search.mapper.pojo.model.spi.PojoRawTypeIdentifier; | ||
|
|
||
| public final class PojoSimpleModelsRawInjectableBinderModel<T> |
There was a problem hiding this comment.
Is the goal of this class to expose only writeable properties?
Can't this be done simply by adding a writeableProperties() method in PojoRawTypeModel? Without necessarily giving these properties a dedicated type, but with a contract that guarantees returned properties can be written to.
Though TBH, I'm not sure why we'd ignore non-writeable properties... if someone starts annotating a getter with @FullTextField inside a binder, I'd rather fail the mapping and tell them they did something wrong.
| @@ -72,6 +76,7 @@ public PropertyMappingDocumentIdOptionsStep documentId() { | |||
| @Override | |||
| public PropertyMappingStep binder(PropertyBinder binder, Map<String, Object> params) { | |||
| children.add( new PropertyBridgeMappingContributor( binder, params ) ); | |||
| injectableBinderCollector.add( binder ); | |||
There was a problem hiding this comment.
If you must do it here, fine, but... any reason you don't do it in org.hibernate.search.mapper.pojo.processing.building.impl.PojoIndexingProcessorPropertyNodeBuilder#propertyBinder instead? Or even in org.hibernate.search.mapper.pojo.mapping.building.impl.PojoIndexModelBinder#bindProperty?
Either would feel like more natural places to collect/inspect binder classes, and you do have access to the introspector there.
There was a problem hiding this comment.
Right 😃 that's why I was asking if it would make sense to "read the binder info" at "binding time" (I think it was on one of the calls we had).
My very first thoughts here were to read the binder metadata prior to building the model. While I couldn't think of an argument why I shouldn't do it just before "injecting" the index fields into the binder (That would be PojoIndexModelBinder#bindProperty from your suggestion) I left it as it was to hear your opinion 😃.
| @Override | ||
| public void inject(Object binder, IndexSchemaElement indexSchemaElement, NamedValues params) { | ||
| IndexFieldReference<F> reference = indexSchemaElement.field( relativeFieldName, typeContributor ).toReference(); | ||
| writeHandle.set( binder, reference ); | ||
| } |
There was a problem hiding this comment.
So this is responsible for applying annotations within a given context and injecting the result into the binder?
Ok, that makes sense to me.
I'm not convinced you need to avoid re-creating injectors at all cost, though. IMO it's fine if you create it at the very last moment, when you create the binders. If performance needs to be improved, you can always add a cache later.
| new KeywordFieldInjectableBinderAnnotationProcessor(); | ||
|
|
||
| @Override | ||
| public InjectableBinderFieldInjector process(PojoInjectablePropertyModel<?> property, KeywordField annotation) { |
There was a problem hiding this comment.
Note the fact that you're manipulating a property model here means we won't be able to use this in javac annotation processors -- because I doubt we can implement PojoPropertyModel on top of annotation processor APIs. I'd love to be proven wrong though.
The javac annotation processor code that infers the index model from annotations will likely be completely separate...
https://hibernate.atlassian.net/browse/HSEARCH-5235
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.