Uploaded image for project: 'Polygene'
  1. Polygene
  2. POLYGENE-257

Custom IdentityGenerator or Serialization hidden by default assemblers

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • None
    • 3.0.0
    • None

    Attachments

      Activity

        niclas Niclas Hedhman added a comment - - edited

        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

        • IdentityGenerator,
        • Serialization
        • and UnitOfWorkFactory
          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:

        • Module.identityGenerator() throws NoSuchServiceException if not found
        • Module.valueSerialization() instantiate OrgJsonValueSerialization
          directly and cache
        • Module.metricsProvider() is package protected, it instantiate
          MetricsProviderAdapter directly and cache

        On develop: metrics have a fallback, the others don't:

        • Module.identityGenerator() throws NoSuchServiceException if not found,
          but is always found (default assemblers)
        • Module.serialization() throws NoSuchServiceException if not found, but
          is always found (default assemblers)
        • Module.metricsProvider() instantiate MetricsProviderAdapter directly
          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:

        Addendum:

        • s/polygene-runtime/polygene-support
        • Tibor, you could try the branch with your application, I hope it'll
          fix your IdentityGenerator issue
        niclas Niclas Hedhman added a comment - - edited 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 IdentityGenerator, Serialization and UnitOfWorkFactory 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: Module.identityGenerator() throws NoSuchServiceException if not found Module.valueSerialization() instantiate OrgJsonValueSerialization directly and cache Module.metricsProvider() is package protected, it instantiate MetricsProviderAdapter directly and cache On develop: metrics have a fallback, the others don't: Module.identityGenerator() throws NoSuchServiceException if not found, but is always found (default assemblers) Module.serialization() throws NoSuchServiceException if not found, but is always found (default assemblers) Module.metricsProvider() instantiate MetricsProviderAdapter directly 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: current `develop` behaviour: https://pasteboard.co/9hJfzPf4r.png with the added `polygene-runtime` layer: https://pasteboard.co/9hJJnHL4h.png with explicit assembly of base services in the application assembly, the `polygene-runtime` layer is not needed so it is not added to the assembly: https://pasteboard.co/9hKatWHMc.png Addendum: s/polygene-runtime/polygene-support Tibor, you could try the branch with your application, I hope it'll fix your IdentityGenerator issue
        niclas Niclas Hedhman added a comment - - edited

        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.

        niclas Niclas Hedhman added a comment - - edited 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.
        eskatos Paul Merlin added a comment -

        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.

        eskatos Paul Merlin added a comment - 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.
        niclas Niclas Hedhman added a comment - - edited
        // 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 );
        
        niclas Niclas Hedhman added a comment - - edited // 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 );
        eskatos Paul Merlin added a comment -
        moduleAssembly.defaultServices()
                .visibleIn( Visibility.application );
        

        is now on develop.

        eskatos Paul Merlin added a comment - moduleAssembly.defaultServices() .visibleIn( Visibility.application ); is now on develop .

        People

          eskatos Paul Merlin
          eskatos Paul Merlin
          Votes:
          0 Vote for this issue
          Watchers:
          2 Start watching this issue

          Dates

            Created:
            Updated:
            Resolved: