[Geoserver-devel] GML binding issues for GEOT-2505

[Justin_Deoliveira]

The discussion is getting a bit much for jira so moving back to the mailing list.

Quoting your last comments ben:

> I tried to fix the encoder binding behaviour about a year ago, using > the technique you advocate. The problem is that the current
> Configuration/Binding/Encoder architecture:

> (1) allows only a single binding for each XSD type QName (specified
> in the first matching Configuration entry),

> (2) requires every binding to be bound to a single XSD type QName
> (returned by the binding getTarget), and

> (3) allows every binding to either be BEFORE/AFTER its parents or
> OVERRIDE *all* of them (returned by the binding getExecutionMode).

> For example, gml:MeasureType is a complexType with simpleContent of
> xs:double. gml:MeasureType is bound to MeasureTypeBinding. xs:double > is bound to XSDoubleTypeBinding. MeasureTypeBinding must be an
> OVERRIDE because, if it has AFTER execution mode, whatever object
> represents a Measure is passed to XSDoubleTypeBinding which cannot
> handle it.

The answer is do not use MeasureTypeBinding since org.geotools.meature.Measure is not the object you want to work with right?

> - If we were able to smuggle a fallback binding for MeasureType into > the binding stack, it would be removed by MeasureTypeBinding being an > OVERRIDE.

The fallback should be a binding higher up in the chain which is type consistent with the object being encoded. Since MEasureTYpeBinding is not type consistent with the objects its execution mode should be completely ignored.

> Even if we changed the encoder to recognise and ignore mismatched
> types and so work with an AFTER execution mode for
> MeasureTypeBinding, there is no type QName to bind generic
> complexType with simpleContent. This pattern has no representation in > the type name hierarchy that drives the construction of the binding
> stack. The Configuration allows only one binding per XSD type name.

Why can't you use xs:anyType which is the root of all types? You are already have a special binding attached to it, why can't that binding handle these other cases as well?

> Please consider your proposal to add a binding for
> AbstractSimpleContentComplexBinding:
> - To what XSD type name should this binding be registered in the
> Configuration?
> - What should AbstractSimpleContentComplexBinding getTarget return (i.e. the XSD type name)?
> - If MeasureTypeBinding enables this binding by using AFTER execution > mode, what stops XSDoubleTypeBinding from being called first? (OK, we > can modify the Encoder to use instanceof checks.)

We could come up with some name, it does not really matter. Then we can have the code that puts together the binding case recognize the simple type complex content case (actually i am pretty sure it already does) and then look for this binding by the special name we decide on.

This name (let's call it xs:simpleTypeWithComplexContent) will be what is returned from getTarget()

MeasureBinding should not factor in at all since its type will be nonsense compared to the object being encoded. It and its execution mode should be ignored.

> How do we register multiple bindings for one XSD type QName and have > them honoured? Perhaps this is the approach we should investigate.
> Perhaps storing lists of bindings in Configurations? This would be a > wide-reaching change. We would need to add a complex-aware version of > every complex binding that we wanted to have ComplexAttribute
> support. Surely this is as bad as an instanceof in each binding.

> Rob's instanceof was allowing bindings to fall back up the Java
> inheritance hierarchy to find bindings the encoder could not handle.

If a bindings declared type hierarchy is not what you need it means don't use that binding. You should completely ditch it. So you should use a configuration that does even register MeasureTypeBinding if at the end of the day you don't want back a org.geotools.

> The current bindings only work, and only in GeoServer, because we put > them at the top of the dependencies in GeoServer WFSConfiguration:
>
> // support GML property type pattern
> Schemas.unregisterComponent(container, XS.ANYTYPE);
> container.registerComponentImplementation(XS.ANYTYPE,
> SubstitutionGroupXSAnyTypeBinding.class);

> This is why the only encoding tests for app-schema are in GeoServer.

Well this is what the system was designed for. If the specialized bindings don't work for you, replace them. I don't see this as a hack, it was how the system was designed to operate.

> Yes, this is horrible. Look at the Configuration dependencies and you > will see it is the only obvious place to smuggle dependency this
> before the stock XSConfiguration without making gt-xsd-core depend on > gt-xsd-gml3. Furthermore, this only works for types that have no
> other bindings so they can fall back to XS.ANYTYPE. There is no XSD
> type name that is the base of complex types and only complex types,
> to which SubstitutionGroupXSAnyTypeBinding could be bound.

> What would work is an encoder that looked at XSD type definitions and > not just XSD type QNames to find bindings. That would allow us to
> have bindings for complex types and complexType-with-simpleContent
> without having to bind to XSD QNames we do not know (defined in user > application schemas). But how to make this not GML specific (i.e.
> retain the generic nature of the Encoder)?

> The current situation is beyond ugly. If you have any better ideas I > would love to hear them.

Unless I am missing something fundamental this should work. I was able to make modifications and get Rob A's original test cases to pass.

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

[Ben_Caradoc-Davies]

On 04/02/10 22:30, Justin Deoliveira wrote:

The answer is do not use MeasureTypeBinding since
org.geotools.meature.Measure is not the object you want to work with right?

Correct. The existing binding configuration maps the XSD type gml:MeasureType to MeasureTypeBinding which encodes a Geotools Measure, but this XSD type can also be represented with a ComplexAttributeImpl.

Why can't you use xs:anyType which is the root of all types? You are
already have a special binding attached to it, why can't that binding
handle these other cases as well?

The problem is that there can only be one[1] binding for any XSD type name. There is already a binding for xs:anyType. We "override" this by smuggling a binding into the GeoServer WFSConfiguration; this is necessary to ensure it is the first xs:anyType binding found. This approach is fragile because it relies on the Configuration dependency search order. I want to fix this.

[1] Where did I put my sword?

MeasureBinding should not factor in at all since its type will be
nonsense compared to the object being encoded. It and its execution mode
should be ignored.

Agreed.

The underlying problem is that encoder bindings are dispatched only on the XSD type, not the type of the object to be encoded. This makes sense if you want to be able to both parse and encode, but does not handle the case of being able to encode disjoint (unrelated) Java types, for example, encoding objects of type Measure and objects of type ComplexAttributeImpl to gml:MeasureType.

We could have some sort of canHandle method on bindings, but this would move a lot of the binding walking logic from the visitor/walker into the bindings, defeating the point of using this pattern.

I think that, for declared-type bindings with a one-to-one Java-to-XSD type mapping, the existing pattern is the right solution. It is fast and clean. We have added on some extra bits to handle ComplexAttributeImpl: I think I need to clean these up.

If a bindings declared type hierarchy is not what you need it means
don't use that binding. You should completely ditch it. So you should
use a configuration that does even register MeasureTypeBinding if at the
end of the day you don't want back a org.geotools.

I do not want to use another configuration: I want to enhance the existing configuration to that it supports these use cases. Rob A and I have discussed several strategies. The one I currently favour is to modify the encoder to recognise special cases, such as bindings that do not have the correct type for the object to be encoded. Or we could even have a parallel dispatch mechanism that is object-type aware.

For example, we could register a component instance in the configuration that provides a factory to create strategies to handle some cases.

This approach would:
- make ComplexAttributeImpl support minimally intrusive by encapsulating all special logic
- be extensible through the configuration rather than encoder modifications
- use all the existing gt-xsd-gml3 bindings for those parts of the tree that require them
- allow me to back out some of the existing workarounds, such as those used to support complex-type-with-simple content
- allow the whole encoder configuration to be used in GeoTools without having to hack GeoServer WFSConfiguration, so I can back out some of those changes as well

I'll see if I can translate my ideas into working code.

Unless I am missing something fundamental this should work. I was able
to make modifications and get Rob A's original test cases to pass.

Did you change the Encoder? Rob A's patch snuck in some method signature changes and inheritance changes that circumvent the binding architecture, in effect disabling MeasureTypeBinding. I do not think this is what you want, as it goes against your stated goal! I am not sure it works the way we want. Please outline how you got it to work.

Kind regards,

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Earth Science and Resource Engineering
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia

[Justin_Deoliveira]

On 2/7/10 11:04 PM, Ben Caradoc-Davies wrote:

On 04/02/10 22:30, Justin Deoliveira wrote:

The answer is do not use MeasureTypeBinding since
org.geotools.meature.Measure is not the object you want to work with
right?

Correct. The existing binding configuration maps the XSD type
gml:MeasureType to MeasureTypeBinding which encodes a Geotools Measure,
but this XSD type can also be represented with a ComplexAttributeImpl.

Why can't you use xs:anyType which is the root of all types? You are
already have a special binding attached to it, why can't that binding
handle these other cases as well?

The problem is that there can only be one[1] binding for any XSD type
name. There is already a binding for xs:anyType. We "override" this by
smuggling a binding into the GeoServer WFSConfiguration; this is
necessary to ensure it is the first xs:anyType binding found. This
approach is fragile because it relies on the Configuration dependency
search order. I want to fix this.

Well the order is dependable in that you can safely override any binding in a dependent configuration. And since XS is depended on by all configurations it should be ok. Where things could conflict is when another configuration depends on your configuration and is trying to do the same thing. But I do not believe that is the case here right?

The alternative is to attach multiple bindings to a single xsd target type. Which might be doable but i don't see much of a benefit over just having a single binding at the top of the chain. In the former you will end up registering an entire new set of bindings for each xsd type.

[1] Where did I put my sword?

MeasureBinding should not factor in at all since its type will be
nonsense compared to the object being encoded. It and its execution mode
should be ignored.

Agreed.

The underlying problem is that encoder bindings are dispatched only on
the XSD type, not the type of the object to be encoded. This makes sense
if you want to be able to both parse and encode, but does not handle the
case of being able to encode disjoint (unrelated) Java types, for
example, encoding objects of type Measure and objects of type
ComplexAttributeImpl to gml:MeasureType.

We could have some sort of canHandle method on bindings, but this would
move a lot of the binding walking logic from the visitor/walker into the
bindings, defeating the point of using this pattern.

I think that, for declared-type bindings with a one-to-one Java-to-XSD
type mapping, the existing pattern is the right solution. It is fast and
clean. We have added on some extra bits to handle ComplexAttributeImpl:
I think I need to clean these up.

Ok, here is where we keep talking past each other. You seem to adamantly be advocating for somehow making the encoder allow a single binding to handle multiple java to xsd type mappings. Whereas I am arguing they should be separate bindings.

Other than the configuration being "fragile" i have still yet to hear why my proposed solution won't work. Apologies, i must be missing something fundamental here.

If a bindings declared type hierarchy is not what you need it means
don't use that binding. You should completely ditch it. So you should
use a configuration that does even register MeasureTypeBinding if at the
end of the day you don't want back a org.geotools.

I do not want to use another configuration: I want to enhance the
existing configuration to that it supports these use cases. Rob A and I
have discussed several strategies. The one I currently favour is to
modify the encoder to recognise special cases, such as bindings that do
not have the correct type for the object to be encoded. Or we could even
have a parallel dispatch mechanism that is object-type aware.

For example, we could register a component instance in the configuration
that provides a factory to create strategies to handle some cases.

This approach would:
- make ComplexAttributeImpl support minimally intrusive by encapsulating
all special logic
- be extensible through the configuration rather than encoder modifications
- use all the existing gt-xsd-gml3 bindings for those parts of the tree
that require them
- allow me to back out some of the existing workarounds, such as those
used to support complex-type-with-simple content
- allow the whole encoder configuration to be used in GeoTools without
having to hack GeoServer WFSConfiguration, so I can back out some of
those changes as well

I'll see if I can translate my ideas into working code.

Unless I am missing something fundamental this should work. I was able
to make modifications and get Rob A's original test cases to pass.

Did you change the Encoder? Rob A's patch snuck in some method signature
changes and inheritance changes that circumvent the binding
architecture, in effect disabling MeasureTypeBinding. I do not think
this is what you want, as it goes against your stated goal! I am not
sure it works the way we want. Please outline how you got it to work.

* Created a special binding capable of parsing and encoding a ComplexAttribute instance
* Attached the binding to XS.anyType
* Modified the encoder to ignore type binding mismatches

Which skips the MeasureTypeBinding all together when the encoder is not actually given a org.geotools.measure.Measure instance.

I think we need to start talking in code. I have the code that I did up for this somewhere. Can you point me at an updated test case of sorts that i can run to test this issue. If not i can try to dig up Rob's original test case.

Kind regards,

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

[Ben_Caradoc-Davies]

On 08/02/10 21:33, Justin Deoliveira wrote:

Well the order is dependable in that you can safely override any binding
in a dependent configuration.

Except bindings defined in XSConfiguration, which include the one we want to override. The constructor for Configuration contains this:

if (!(this instanceof XSConfiguration)) {
     dependencies.add(new XSConfiguration());
}

So every Configuration, such as WFSConfiguration, will get the original XSConfiguration as its first dependency, so if you override the binding for xs:anyType in GMLConfiguration, and WFSConfiguration depends on GMLConfiguration, you still get the original binding for xs:anyType because WFSConfiguration gets a dependency on the original XSConfiguration ahead of GMLConfiguration. In short: overrides of XSConfiguration bindings are not honoured in dependencies.

The only reason GeoServer app-schema encoding works now is because I put an explicit override for xs:anyType in GeoServer WFS 1.1 WFSConfiguration because the one I tried in GMLConfiguration (a dependency) was ignored.

Fixing this requires both:
(1) removing the XSConfiguration automatic dependency from the constructor of Configuration, and
(2) going through all the existing subclasses of Configuration and adding an explicit dependency on XSConfiguration for those that need one.

This is a backwards-incompatible change and will break third party code that assumes an automatic dependency on XSConfiguration.

- If you agree with this change, I am happy to do the work and make the change.

- If you are not happy with this change and would rather leave things as they are, that is fine too. Users just need to be add a binding override in any configuration that uses GMLConfiguration. A minor inconvenience.

An even better idea: add all Configuration dependencies in reverse order [.add(0,...)] to ensure XSConfiguration is last on the list! That might work. We might run into problems if there are any existing ordering assumptions.

Ok, here is where we keep talking past each other. You seem to adamantly
be advocating for somehow making the encoder allow a single binding to
handle multiple java to xsd type mappings. Whereas I am arguing they
should be separate bindings.

I am not adamantly for anything. I am just trying to find enough wiggle room to encode a ComplexAttribute without making a big mess of the encoder codebase.

Other than the configuration being "fragile" i have still yet to hear
why my proposed solution won't work. Apologies, i must be missing
something fundamental here.

The only thing I meant by "fragile" is the behaviour I indicated above: overrides of XSConfiguration work only when defined in the top-level configuration, but are unexpectedly ignored when defined in a dependency. This forces users of a configuration to redefine any XSConfiguration overrides in their dependencies, and thus be aware of the implementation of their dependencies. I think it is fair to call this "fragile". This is not a personal criticism, just my opinion as a consumer of this class.

Unless I am missing something fundamental this should work. I was able
to make modifications and get Rob A's original test cases to pass.

Please outline how you got it to work.

* Created a special binding capable of parsing and encoding a
ComplexAttribute instance
* Attached the binding to XS.anyType
* Modified the encoder to ignore type binding mismatches

What modifications did you make to the encoder?

I have tried a few approaches and I can't find an easy way to make this change. The problem I keep running into is that BindingWalker builds the binding execution chain from the XSD type hierarchy and throws away the xs:anyType binding if the final binding is an OVERRIDE. For example, If the XSD type is gml:MeasureType, only MeasureTypeBinding is left on the binding execution chain. The binding for xs:anyType is gone.

Perhaps we should leave all bindings on the chain and change the walking logic? Is this what you did? Or can we hardcode a fallback to xs:anyType if there are no matching bindings?

I am experimenting ...

The other problem is that the visitor has no type information about the object being encoded. The binding chain is built before the type of the object to be encoded is known. Did you change the visitor interface? Where to you have "object instanceof binding.getType()"?

I think we need to start talking in code. I have the code that I did up
for this somewhere. Can you point me at an updated test case of sorts
that i can run to test this issue. If not i can try to dig up Rob's
original test case.

One of my original patches contains fixed versions of Rob's unit tests:
http://jira.codehaus.org/secure/attachment/47249/GEOT-2505.xsd-gml3.patch
After applying this patch (which also updates GMLSchema), revert the binding changes, as I think we have decided not to use them.

Thanks again Justin, for your help and your patience.

Kind regards,

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Earth Science and Resource Engineering
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia

[Ben_Caradoc-Davies]

On 10/02/10 13:25, Ben Caradoc-Davies wrote:

Perhaps we should leave all bindings on the chain and change the walking
logic? Is this what you did? Or can we hardcode a fallback to xs:anyType
if there are no matching bindings?
I am experimenting ...
The other problem is that the visitor has no type information about the
object being encoded. The binding chain is built before the type of the
object to be encoded is known.

I think I have a solution: a new visitor that finds any mismatched bindings. If any are found, call the xs:anyType binding directly with executor.visit(binding), otherwise let the executor do the walking as usual. This seems to work. (Still debugging unit test failures.)

Is this approach acceptable?

"object instanceof binding.getType()"?

Oops: should be "binding.getType().isAssignableFrom(object.getClass())"

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Earth Science and Resource Engineering
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia

[Justin_Deoliveira]

On 2/10/10 12:25 AM, Ben Caradoc-Davies wrote:

On 08/02/10 21:33, Justin Deoliveira wrote:

Well the order is dependable in that you can safely override any binding
in a dependent configuration.

Except bindings defined in XSConfiguration, which include the one we
want to override. The constructor for Configuration contains this:

if (!(this instanceof XSConfiguration)) {
      dependencies.add(new XSConfiguration());
}

So every Configuration, such as WFSConfiguration, will get the original
XSConfiguration as its first dependency, so if you override the binding
for xs:anyType in GMLConfiguration, and WFSConfiguration depends on
GMLConfiguration, you still get the original binding for xs:anyType
because WFSConfiguration gets a dependency on the original
XSConfiguration ahead of GMLConfiguration. In short: overrides of
XSConfiguration bindings are not honoured in dependencies.

The only reason GeoServer app-schema encoding works now is because I put
an explicit override for xs:anyType in GeoServer WFS 1.1
WFSConfiguration because the one I tried in GMLConfiguration (a
dependency) was ignored.

Fixing this requires both:
(1) removing the XSConfiguration automatic dependency from the
constructor of Configuration, and
(2) going through all the existing subclasses of Configuration and
adding an explicit dependency on XSConfiguration for those that need one.

Yes, this is an issue. I think the right way to solve it is to ensure that Configuration.allDependencies() returns the list in a way that maintains the order. To do this robustly I think we might have to actually build the dependency graph and traverse it to build a order list of dependencies.

This is a backwards-incompatible change and will break third party code
that assumes an automatic dependency on XSConfiguration.

Well the current order does not really make any sense so i am confident that no client code is depending on it. The only assumption made afaik is that the "root" configuration is the last to register bindings, which will be maintained.

- If you agree with this change, I am happy to do the work and make the
change.

- If you are not happy with this change and would rather leave things as
they are, that is fine too. Users just need to be add a binding override
in any configuration that uses GMLConfiguration. A minor inconvenience.

An even better idea: add all Configuration dependencies in reverse order
[.add(0,...)] to ensure XSConfiguration is last on the list! That might
work. We might run into problems if there are any existing ordering
assumptions.

This might work, but I am not sure works in every case. I still think there is the possibility of an improper ordering resulting in a dependent configuration not able to override bindings of a configuration it depends on.

Ok, here is where we keep talking past each other. You seem to adamantly
be advocating for somehow making the encoder allow a single binding to
handle multiple java to xsd type mappings. Whereas I am arguing they
should be separate bindings.

I am not adamantly for anything. I am just trying to find enough wiggle
room to encode a ComplexAttribute without making a big mess of the
encoder codebase.

Other than the configuration being "fragile" i have still yet to hear
why my proposed solution won't work. Apologies, i must be missing
something fundamental here.

The only thing I meant by "fragile" is the behaviour I indicated above:
overrides of XSConfiguration work only when defined in the top-level
configuration, but are unexpectedly ignored when defined in a
dependency. This forces users of a configuration to redefine any
XSConfiguration overrides in their dependencies, and thus be aware of
the implementation of their dependencies. I think it is fair to call
this "fragile". This is not a personal criticism, just my opinion as a
consumer of this class.

Not taking any personal criticism, just trying to find a solution that works for everybody. And yes what you illustrate above is an issue and one I think we can agree needs to be solved asap. So let's move forward with that.

Unless I am missing something fundamental this should work. I was able
to make modifications and get Rob A's original test cases to pass.

Please outline how you got it to work.

* Created a special binding capable of parsing and encoding a
ComplexAttribute instance
* Attached the binding to XS.anyType
* Modified the encoder to ignore type binding mismatches

What modifications did you make to the encoder?

I have tried a few approaches and I can't find an easy way to make this
change. The problem I keep running into is that BindingWalker builds the
binding execution chain from the XSD type hierarchy and throws away the
xs:anyType binding if the final binding is an OVERRIDE. For example, If
the XSD type is gml:MeasureType, only MeasureTypeBinding is left on the
binding execution chain. The binding for xs:anyType is gone.

Perhaps we should leave all bindings on the chain and change the walking
logic? Is this what you did? Or can we hardcode a fallback to xs:anyType
if there are no matching bindings?

I am experimenting ...

The other problem is that the visitor has no type information about the
object being encoded. The binding chain is built before the type of the
object to be encoded is known. Did you change the visitor interface?
Where to you have "object instanceof binding.getType()"?

I think we need to start talking in code. I have the code that I did up
for this somewhere. Can you point me at an updated test case of sorts
that i can run to test this issue. If not i can try to dig up Rob's
original test case.

One of my original patches contains fixed versions of Rob's unit tests:
http://jira.codehaus.org/secure/attachment/47249/GEOT-2505.xsd-gml3.patch
After applying this patch (which also updates GMLSchema), revert the
binding changes, as I think we have decided not to use them.

Thanks again Justin, for your help and your patience.

Cool, thank you for your patience I will dig up the code I wrote and try to make sense of it. Thanks for pointing me at that test case.

Kind regards,

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

[Ben_Caradoc-Davies]

On 10/02/10 21:51, Justin Deoliveira wrote:

Yes, this is an issue. I think the right way to solve it is to ensure
that Configuration.allDependencies() returns the list in a way that
maintains the order. To do this robustly I think we might have to
actually build the dependency graph and traverse it to build a order
list of dependencies.

Dependency order is used for two things:
(1) binding resolution, when it is walked in order, and
(2) binding extraction / component configuration, when it is walked in reverse order so that higher priority bindings are extracted later and override lower priority bindings when extracted into a Map. This order is also used for component configuration.

The problem is that we are flattening a tree into a list with no duplicates. If we do so, any otherwise repeated dependency will be be out of order in either the forward or reverse direction. For example, suppose B depends on A, C depends on A, and D depends on B and C.

Depth-first binding resolution order:
[D, B, A, C]

Strict-dependency-order binding resolution:
[D, B, C, A]

My patch GEOT-2505.anytype.patch imposes depth-first binding resolution and chooses reverse-resolution-order for extraction, that is option [D, B, A, C] above. The obvious problem is that C is configured before A (configuration is in reverse order), which might lead to NPEs because of missing components.

Strict-dependency-order is also problematic because if we later have E that depends on A and D you would get

[E, D, B, C, A]

and are effectively allowing C to override bindings in a direct top-priority dependency. E would have to override bindings in C even though it depends directly on A. Perhaps this is a good thing.

Are we saying dependencies have no order or priority? Is so, then what is an override? How do we determine which override wins?

Depth-first is nice because it is a predictable, obvious order, supports overriding. And overrides of overrides. And can be implemented by recursion.

This might work, but I am not sure works in every case. I still think
there is the possibility of an improper ordering resulting in a
dependent configuration not able to override bindings of a configuration
it depends on.

I think you are right. In the depth-first case, C cannot override A. Maybe GEOT-2948 is not superseded after all ...

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Earth Science and Resource Engineering
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia

[Ben_Caradoc-Davies]

On 15/02/10 14:21, Ben Caradoc-Davies wrote:

I think you are right. In the depth-first case, C cannot override A.

I have convinced myself that we have to use strict dependency order. I have updated my GEOT-2505 patch to implement this.

Justin, I hope very much that this is the final resolution of this long running issue. Please review the two remaining patches.

Kind regards,

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Earth Science and Resource Engineering
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia

[Justin_Deoliveira]

On 2/15/10 2:42 AM, Ben Caradoc-Davies wrote:

On 15/02/10 14:21, Ben Caradoc-Davies wrote:

I think you are right. In the depth-first case, C cannot override A.

I have convinced myself that we have to use strict dependency order. I
have updated my GEOT-2505 patch to implement this.

Justin, I hope very much that this is the final resolution of this long
running issue. Please review the two remaining patches.

I am not sure why the patch I proposed for dependency order won't work? Yes the solution you proposed will probably work in most cases but it is not fully robust, whereas building the dependency graph and processing it in an order that maintains that no configuration is processed before a configuration it depends on is.

Yes the initial issue caused a test failure but a minor special case check fixes that. So unless you can come up with a reason not to use it I would like to stick with it.

As for the rest of the patch I think it is acceptable. I still think this logic could have been rolled into the main binding walking logic but the new patches are a step up as they isolate the special case check to a single class.

So +1 on the patches minus the configuration dependency stuff. Unless you can illustrate why the updated patch on GEOT-2948 won't work I would like to stick with that approach. Apologies if i am missing something.

Also can you keep improvements such as cleaning up generics out of sensitive patches such as this. It is tricky enough to review the patch in detail without having to worry about all the trivial changes.

Thanks for your continued patience Ben.

-Justin

Kind regards,

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

[Ben_Caradoc-Davies]

On 15/02/10 23:24, Justin Deoliveira wrote:

Yes the initial issue caused a test failure but a minor special case
check fixes that. So unless you can come up with a reason not to use it
I would like to stick with it.

I am happy to use your updated patch (GEOT-2948-2.patch). I have added a one line bugfix for a Java 5 defect (plus a three-line comment), as described in:
http://jira.codehaus.org/browse/GEOT-2948

With the additional bugfix, your patch works.

So +1 on the patches minus the configuration dependency stuff. Unless
you can illustrate why the updated patch on GEOT-2948 won't work I would
like to stick with that approach. Apologies if i am missing something.

You are not missing anything. I will use your approach.

Also can you keep improvements such as cleaning up generics out of
sensitive patches such as this. It is tricky enough to review the patch
in detail without having to worry about all the trivial changes.

Yes, I was a bit naughty. I have reverted my Configuration changes (including the spurious preening), applied GEOT-2948-2.patch plus the Java 5 bugfix, and committed.

Thanks for your continued patience Ben.

And thanks very much for all your work on this issue.

Kind regards,

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Earth Science and Resource Engineering
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia