Comments inline.... snip...
> > and that's OK I think :) as binding.ws and binding.jsonrpc could end up > on different port numbers for example (depending on their node > configuration). And they might not. > > Actually, I was struggling to understand why we needed this test for > duplicate names at all. > > Does the spec forbids two bindings with the same name? No but it says that if the name is used as the default binding URI then only one binding for a service can use this default. > > By trying hard to detect duplicate names early on (without having all > the info about what the binding URIs will end up to be), won't we raise > errors even for cases that should work? > I agree that we don't have all of the information required to carry out this test. Hence the TODO in the code. > One example of the issues I was running into: > <service...> > <binding.jsonrpc/> > <binding.ws uri="/whatever"> > </service> > throwing a duplicate name exception as the two bindings had the same name. > > On a fun note, I think that: > <service name="aservice"> > <binding.jsonrpc name="whatever"/> > <binding.ws uri="/whatever"> > </service> > would probably not have thrown an exception, but should have :) > Hmmm. not sure but you could be right. > So my recommendation would be to not try too hard to detect these > duplicates early on based on binding names (as the detection algorithm > is too fragile at that point), instead detect duplicates at the very end > when we see that we end up with duplicate URIs. +1. I'll add a new method to scan the model for duplicate URI that can be called independently. > > > > > > Also can you give a quick example the problems you found relating to > "avoid > > adding binding name to itself, and consider binding URI when specified > in > > the composite in the single service case too" so I can understand the > other > > code changes. > > One issue was that the code was not considering the binding URI when > there was only one service on a component, for example IIRC: > <component name="Store"> > <service name="Widget"> > <binding.http uri="/ui"/> > </service> > </component> > was bound to /Store (the component URI) instead of /ui. > > I understand that the binding name should be omitted from the computed > URI in the single service case, but the binding URI should be considered > if specified, leading to the requirement to distinguish between binding > name (always know, not always considered) and binding URI (not always > specified, always considered when specified). Ok. Thank you. I had interpreted the spec incorrectly here. I had mistakenly read assembly spec line 2375 as "Where a component has only a single service, the value of the Service Binding URI is null" instead of what it actually says, which is "Where a component has only a single service, the * default* value of the Service Binding URI is null". > > About the "adding the binding name to itself" part, this is a little > complicated. IIRC the issue was that CompositeConfigurationBuilder would > first convert: > <component name="Foo"> > <service...> > <binding.sca/> > </service> > </component> > > into: > <component name="Foo"> > <service...> > <binding.sca uri="Foo"/> > </service> > </component> > > Then when an SCANode would consume that, CompositeBuilder.build would > turn it into: > <component name="Foo"> > <service...> > <binding.sca uri="Foo/Foo"/> > </service> > </component> > > as I think the old CompositeConfigurationBuilder code used in that case > used to concatenate the component URI and the binding URI. OK, I see. So this was a combination of the old and the new causing problems. I see that you now call the old after the new has run so am assuming that your changes are making this behave now. Let me know if not. > > > > > Thanks > > > > Simon > > > > -- > Jean-Sebastien > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [EMAIL PROTECTED] > For additional commands, e-mail: [EMAIL PROTECTED] > >
