Details
-
Bug
-
Status: Closed
-
Major
-
Resolution: Fixed
-
None
-
None
Description
Attachments
Activity
My response to that, effectively saying; Let;s not make large change;
Very good analysis, Paul!
And you are right about the difference between "Module Default" and
"Fallback" differentiation... And what matters now is the way to implement
it. I am not convinced that a default layer is the way to go, although it
seems to be the most easy way to implement it. As a start, I have no
recollection that defined semantics of multiple reachable layers are
anything but "Ambiguous Type". In Visibility.layer, multiple visible
Composites in same layer would result AmbiguousTypeException, and I suspect
that currently that is also the case if found in multiple layers. And for
your approach to work, that would need to be changed to some type of
ordering/priority of layers, or special handling of the polygene-runtime
layer. Neither seems tempting to me right now (before coffee). That change
is also non-reversible in a compatible manner...
What other choices are there?
We could remove the said default types, and instead do the equivalent in
AbstractPolygeneTest (maybe one of the super classes), where it is really
convenient and doesn't impact production code. That would be the quickest
route for a 3.0 and keep the door open for compatible changes in 3.1
We could introduce something like;
module.asFallbackModule().visibleIn( Visibility.application);
where the semantics are; If no type is found in layer, check the default
module of the layer. If no type found in underlying layers, check default
modules in those underlying layers.
But then again, WHY? Why have two priority levels in TypeLookUp? Why not
have N levels of priority? And that question to me sounds like a slippery
slope, and I would prefer to not go there.
So, right now (half way through coffee cup), I don't think we need anything
conceptually new, just
1. Remove IdentityGenerator and Serialization from defaultAssemblers
2. Change AbstractPolygeneTest to add those by default.
Yeah, the added magic layer is not a nice solution. Or it would need to be read-only and hidden. Which sounds very ad-hoc.
I agree we should remove both IdentityGenerator and Serialization from defaultAssemblers.
But I think we need to provide some convenience for both tests and applications assemblies. It basically should be an opt-in. For AbstractPolygeneTest it could be enabled by default with a way to opt-out.
I also think we should have "default services", but no "fallbacks", and treat MetricsProvider the same way. So that the service semantics can apply directly.
For both applications and non-trivial tests one will want to opt-in on a given Layer/Module. Moreover, one may want the default IdentityGenerator but not the default Serialization etc...
Here are a few assembly api proposals:
// on the ModuleAssembly: moduleAssembly.defaultServices() .visibleIn( Visibility.application ); // for each "default service": moduleAssembly.defaultIdentityGenerator() .visibleIn( Visibility.application ); moduleAssembly.defaultSerialization() .visibleIn( Visibility.application ); moduleAssembly.defaultMetricsProvider() .visibleIn( Visibility.application );
These would return a classic `ServiceDeclaration`. It'd be easily usable with SingletonAssembler, tests etc.. while providing fine grained control for non trivial assemblies.
On a side note, ideally, service lookup exceptions should guide the user towards resolving her assembly errors. For now they only say, not found, here are the ones that you could have found. That's not very useful. For injections, knowing what required a service without having to "decypher" the stacktrace would be way better.
// on the ModuleAssembly:
moduleAssembly.defaultServices()
.visibleIn( Visibility.application );
I am Ok with the above.
But for
moduleAssembly.defaultIdentityGenerator() .visibleIn( Visibility.application );
I think the convenience is too marginal compared with;
new IdentityGeneratorAssembler()
.visibleIn( Visibility.application )
.assemble( module );
or even
module.services(IdentitityGenerator.class) .withMixin( DefaultIdentityGeneratorMixin.class ) .visibleIn( Visibility.application );
moduleAssembly.defaultServices() .visibleIn( Visibility.application );
is now on develop.
Paul's analysis of the situation (from mail thread);
The issue is not about type hierarchy but structure and visibility.
Default assemblers are added to ALL modules for
if no service assignable to each was assembled.
This makes sense for UnitOfWorkFactory, UnitOfWorkFactory is bound to
its module and one always create an uow from a given
module.
But it doesn't for the two other types for which one will want to get
the closest in the app structure instead.
I think this mixes two use cases.
Assembly of all modules must contain a service of a given type. That's
the UnitOfWorkFactory case and current
"default assemblers" do the job.
If a service of a given type cannot be found, then fallback to a
framework default. That's the IdentityGenerator and
Serialization cases. Both because they are core api services (e.g. in
Module) and almost all extensions and a bunch
of libraries use them. But assembling them in all modules prevent
reasonable assemblies, as I see it it's a
regression. And requiring users to assemble some special services in all
their modules feels wrong.
In 2.1, serialization and metrics have a fallback, identity generation
doesn't:
directly and cache
MetricsProviderAdapter directly and cache
On develop: metrics have a fallback, the others don't:
but is always found (default assemblers)
is always found (default assemblers)
and cache
but the first two are assembled in all modules anyway.
A noop implementation is a good fit for metrics.
UuidGeneratorMixin is core and a simple class so it could easily be
instantiated directly as a fallback.
Our default serialization requires composition but it could be reworked
to be instantiatable, requiring injection.
But these services are not bound to their assembly module, so they could
be shared across modules.
Declaration in all modules is cheap, but it pollutes the application
model in all modules, which is not nice.
And returning plain Objects where one would expect services feels weird
to me. Plus it makes getting these core
services injected in composites impossible.
If my assembly misses these core services, it's very easy to fall into
the cryptic assembly error trap.
In most cases you want to fix the assembly issue holistically according
to your application needs.
That's how 2.x works.
The intent of "default assemblers" for IdentityGenerator and
Serialization is convenience.
When starting an application or writing Polygene tests one wants simple
defaults.
But the way they are done now gets in the way when assembling non
trivial applications.
One way to provide these fallbacks while respecting the structure and
visibility concept, for the sake of least
surprise, would be to add an arbitrary layer with these default services
with application visibility and make all
other "leaf" layers use this arbitrary layer. Effectively making these
default services visible to the whole
application while allowing any custom implementations assembled to
override them.
We could also add a way to opt-out of this during assembly, for a
stricter structure.
Metrics could then work the same way for the sake of least surprise
again, still providing a noop implementation.
I gave the idea a try in the `pm/bootstrap/support-layer` branch, it
passes the whole test suite.
I opened a GitHub PR so it's easier to look at:
https://github.com/apache/polygene-java/pull/6/files
The controversial aspect of this solution is that it automatically adds
an arbitrary layer to support Polygene
core services that an application can override.
But I feel that's far less invasive than adding several assembly
declarations to all modules.
And it makes structure reflect the real intent.
Here are some Envisage screenshots to illustrate the change:
https://pasteboard.co/9hJJnHL4h.png
the `polygene-runtime` layer is not needed so
it is not added to the assembly: https://pasteboard.co/9hKatWHMc.png
Addendum:
fix your IdentityGenerator issue