jax-rs-spec
  1. jax-rs-spec
  2. JAX_RS_SPEC-383

How do Features effect Configuration.getInstances() and getClasses()?

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Invalid
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: None
    • Labels:
      None

      Description

      Do Configuration.getClasses() and getInstances() return providers registered through features? I would say no, they shouldn't. Otherwise any implementation of replaceWith() might get some big weirdness of double-registering of provider instances and weird behavior.

        Activity

        Hide
        Marek Potociar added a comment -

        Yes, unless the features have not been configured by the runtime. The replaceWith() is going to be removed from API.

        Show
        Marek Potociar added a comment - Yes, unless the features have not been configured by the runtime. The replaceWith() is going to be removed from API.
        Hide
        Marek Potociar added a comment -

        Resolved (hopefully)

        Show
        Marek Potociar added a comment - Resolved (hopefully)
        Hide
        patriot1burke added a comment -

        There is still a problem related to issue 382. If Configuration.getClasses() and getInstances() return providers registered by features then you have some potential weird bugs where a provider is doubly registered when calling ClientBuilder.withConfig().

        Show
        patriot1burke added a comment - There is still a problem related to issue 382. If Configuration.getClasses() and getInstances() return providers registered by features then you have some potential weird bugs where a provider is doubly registered when calling ClientBuilder.withConfig().
        Hide
        Marek Potociar added a comment -

        That's something feature should take care of. That's btw also why a feature needs to know that such provider has been already registered by another feature. FWIW, in Jersey we detect these double registrations and log warnings. Also our features are coded defensively in this regard.

        Show
        Marek Potociar added a comment - That's something feature should take care of. That's btw also why a feature needs to know that such provider has been already registered by another feature. FWIW, in Jersey we detect these double registrations and log warnings. Also our features are coded defensively in this regard.
        Hide
        patriot1burke added a comment -

        You can't really code defensively for this particular use case. This is how withConfig() would be implemented.

              setProperties(config.getProperties());
              for (Class clazz : config.getClasses())
              {
                 Map<Class<?>, Integer> contracts = config.getContracts(clazz);
                 register(clazz, contracts);
              }
              for (Object obj : config.getInstances())
              {
                 Map<Class<?>, Integer> contracts = config.getContracts(obj.getClass());
                 register(obj, contracts);
              }
        

        How exactly would a Feature code defensively for this? It can't. There's no way to delete a registered component, so there would be no way for the Feature to override the already registered one. Plus, because getInstances() returns a set, there's no guaranteed order of instances either so the feature may not even be able to detect it needs to override.

        I don't know a solution for this problem.

        Show
        patriot1burke added a comment - You can't really code defensively for this particular use case. This is how withConfig() would be implemented. setProperties(config.getProperties()); for ( Class clazz : config.getClasses()) { Map< Class <?>, Integer > contracts = config.getContracts(clazz); register(clazz, contracts); } for ( Object obj : config.getInstances()) { Map< Class <?>, Integer > contracts = config.getContracts(obj.getClass()); register(obj, contracts); } How exactly would a Feature code defensively for this? It can't. There's no way to delete a registered component, so there would be no way for the Feature to override the already registered one. Plus, because getInstances() returns a set, there's no guaranteed order of instances either so the feature may not even be able to detect it needs to override. I don't know a solution for this problem.
        Hide
        Marek Potociar added a comment -

        I have not received any objections from you to my last reply in our EG mailing list conversation, so I'm assuming I can close the issue, right? In summary:

        To implement the withConfig(Configuration) one does not need to know which providers are registered by a user directly and which are registered via feature. It is enough to know which features have already been enabled and this information is already available in the API.

        Show
        Marek Potociar added a comment - I have not received any objections from you to my last reply in our EG mailing list conversation, so I'm assuming I can close the issue, right? In summary: To implement the withConfig(Configuration) one does not need to know which providers are registered by a user directly and which are registered via feature. It is enough to know which features have already been enabled and this information is already available in the API.
        Hide
        patriot1burke added a comment -

        (From my email response)

        Ok, i'll try one last time to explain...

        Client client = ClientBuilder.newBuilder().register(MyFeature.class).build();

        MyFeature does:

        featureContext.register(new MyFilter(random));

        Next this happens:

        ClientBuilder.newBuilder().withConfig(client.getConfiguration());

        1) client.getConfiguration().getInstances() will return an instance MyFilter
        2) MyFeature is enabled.

        So, if you implement withConfig() with no special implementation specific code, just using the Configuration interface, there is no way for it to know that MyFilter was registered by MyFeature. So withConfig() would do:

        this.register(myFilterInstance);
        this.register(MyFeature.class);

        And you'd have myFilterInstance registered twice. MyFeature may be written with the assumption that instances it creates and registered are not shared and the above scenario may create unforseen problems and bugs.

        I'm fine with having to write a proprietary, implementation specific interface to solve this problem. I just thought I'd make it clear that Configuration as-is, cannot be solely used to implement withConfig().

        Show
        patriot1burke added a comment - (From my email response) Ok, i'll try one last time to explain... Client client = ClientBuilder.newBuilder().register(MyFeature.class).build(); MyFeature does: featureContext.register(new MyFilter(random)); Next this happens: ClientBuilder.newBuilder().withConfig(client.getConfiguration()); 1) client.getConfiguration().getInstances() will return an instance MyFilter 2) MyFeature is enabled. So, if you implement withConfig() with no special implementation specific code, just using the Configuration interface, there is no way for it to know that MyFilter was registered by MyFeature. So withConfig() would do: this.register(myFilterInstance); this.register(MyFeature.class); And you'd have myFilterInstance registered twice. MyFeature may be written with the assumption that instances it creates and registered are not shared and the above scenario may create unforseen problems and bugs. I'm fine with having to write a proprietary, implementation specific interface to solve this problem. I just thought I'd make it clear that Configuration as-is, cannot be solely used to implement withConfig().
        Hide
        Marek Potociar added a comment -

        I can only repeat myself and ask once again the questions that you did not answer in our email conversation so far:
        Why do you need to know which provider was registered by which feature? Configuration API provides you ways of finding out which features have already been enabled. You do not need to enable the feature again, if it's already enabled.

        Show
        Marek Potociar added a comment - I can only repeat myself and ask once again the questions that you did not answer in our email conversation so far: Why do you need to know which provider was registered by which feature? Configuration API provides you ways of finding out which features have already been enabled. You do not need to enable the feature again, if it's already enabled.
        Hide
        patriot1burke added a comment -

        Ok, I think I get your point now. withConfig() does not need to execute the feature if it is already enabled in the original Configuration. getInstances() will be shared between the source configuration and the new ClientBuilder, which is not ideal, but I'm fine with that constraint.

        Thanks for your patience.

        Show
        patriot1burke added a comment - Ok, I think I get your point now. withConfig() does not need to execute the feature if it is already enabled in the original Configuration. getInstances() will be shared between the source configuration and the new ClientBuilder, which is not ideal, but I'm fine with that constraint. Thanks for your patience.

          People

          • Assignee:
            Marek Potociar
            Reporter:
            patriot1burke
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: