On 3/10/2013 9:19 AM, Marek Potociar wrote:
>
> On Mar 9, 2013, at 2:02 PM, Bill Burke <bburke_at_redhat.com> wrote:
>
>>
>>
>> On 3/9/2013 4:10 AM, Marek Potociar wrote:
>>> On 9. 3. 2013, at 0:19, Bill Burke <bburke_at_redhat.com> wrote:
>>>
>>>>
>>>>
>>>> On 3/8/2013 5:42 PM, Marek Potociar wrote:
>>>>>
>>>>> On Mar 8, 2013, at 11:34 PM, Bill Burke <bburke_at_redhat.com> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On 3/8/2013 12:25 PM, Marek Potociar wrote:
>>>>>>>
>>>>>>> On Mar 8, 2013, at 4:52 PM, Marek Potociar <marek.potociar_at_oracle.com> wrote:
>>>>>>>
>>>>>>>>
>>>>>>>> On Mar 7, 2013, at 11:34 PM, Bill Burke <bburke_at_redhat.com> wrote:
>>>>>>>>
>>>>>>>>> #1 Do you really want somebody calling replaceWith() within Feature.configure() or DynamicFeature.configure()? IMO, replaceWith() should be moved to ClientBuilder, Client and WebTarget and not be part of Configurable. (Or even removed from the API and deferred to a later release)
>>>>>>>>
>>>>>>>> I'm fine with making it part of client component APIs. I'm strongly against removing from the API. Your issue has been targeted for 2.0.
>>>>>>>
>>>>>>> I gave it another look and I'm actually not against against removing the replaceWith() altogether. I could not find any use case in our code to justify keeping it there. Of course, we would have to keep some similar method on the ClientBuilder API.
>>>>>>>
>>>>>>> I'm going to remove the replaceWith() method completely (sans new method on ClientBuilder) unless anybody objects...
>>>>>>
>>>>>> I talked about this in JIRA too.
>>>>>>
>>>>>> 1) Invocation.Builder still needs to set properties whether it implements Configurable or not
>>>>>>
>>>>>> 2) ClientBuilder.withConfig(Configuration) still has issues. If Configuration.getInstances() and getClasses() returns any providers registered by Features, they you have some potentially weird problems of a provider being doubly registered.
>>>>>>
>>>>>> For example, if you call this in a feature:
>>>>>>
>>>>>> configurable.register(MyFilter.class, Priorities.AUTHENTICATION);
>>>>>
>>>>> You should code your features defensively and also take care of it in your runtime impl. IMO. Features need to know what other features registered as providers & properties etc. Also users should be able to know what everything is configured in the system IMO.
>>>>
>>>> You can't really code defensively for this particular case. This is how withConfig() would be implemented.
>>>>
>>>> {code}
>>>> 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);
>>>> }
>>>> {code}
>>>>
>>>> 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.
>>>
>>> 1. register(feature) doesn't mean that feature.configure(...) is invoked immediately. It's your impl. detail if you are doing it.
>>
>> Client client = ClientBuilder.newBuilder().register(new Feature()).build();
>>
>> Response response = client.target("").request().get();
>>
>> // Feature.configure() has been called
>> Client client2 = ClientBuilder.newBuilder().withConfig(client.getConfiguration());
>
> In your withConfig() implementation you can consult config.isEnabled(...) for any feature returned from config.getClasses() and config.getInstances() and then internally register this feature "as enabled" to prevent multiple registrations.
>
How can withConfig() know which providers a feature has registered? It
can't...
>> An even better example is the server's Configuration which can continuously be used to create Clients at runtime.
>
> Same solution as above.
>
>>
>> I'm fine with making it an implementation detail and instead have my code do a typecast of Configuration within withConfig() to an implementation specific interface
>>
>> {code}
>> ResteasyConfiguration configParam = (ResteasyConfiguration)configParam;
>> {code}
>>
>> But, you need to agree that Configuration, by itself, as-is, can't be used solely to implement withConfig().
>
> Still not sure why I should agree with such statement. From the current Configuration API you know:
>
> 1. what properties are set
> 2. what provider and feature classes are registered
> 3. which provider/feature class is registered for which contracts and with what priority
> 4. what features have been enabled (so far)
>
> What else do you need to know to implement ClientBuilder.withConfig(Configuration)?
>
What else? Whether a provider has been registered by a feature or by the
user.
Like I said, I'm fine doing something implementation dependent, but the
Configuration interface can't be used as-is to implement withConfig().
--
Bill Burke
JBoss, a division of Red Hat
http://bill.burkecentral.com