Hi,
today I was trying to port over an ebRIM record implementation we made some time
ago and, besides the several difficulties caused by the many changes to
the base classes I’ve found that the core CSW classes regressed and cannot
tell apart anymore two record types with the same name, but different namespace
(we have a custom ebRim record that uses a different namespace and uses extensions, in parallel
with the standard one):
http://jira.codehaus.org/browse/GEOS-6368
This is due to the extensive usage of getLocalName() in the current implementation,
for example:
https://github.com/geoserver/geoserver/blob/2.4.x/src/extension/csw/api/src/main/java/org/geoserver/csw/store/AbstractCatalogStore.java#L49
https://github.com/geoserver/geoserver/blob/2.4.x/src/extension/csw/api/src/main/java/org/geoserver/csw/store/CatalogStoreCapabilities.java
Niels, I’m surprised that a developer with an app-schema background would go there,
so I guess there are good reasons for this change, and I’m just not seeing them?
Regardless, this is a regression and needs to be fixed, but I would like to understand what
challenges got us here to avoid exchanging one regression with another
Cheers
Andrea
–
== Our support, Your Success! Visit http://opensdi.geo-solutions.it for more information ==
Ing. Andrea Aime
@geowolf
Technical Lead
GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549
http://www.geo-solutions.it
http://twitter.com/geosolutions_it
Hello Andrea,
All the changes that were made to those base classes were done to make those classes generic for different record types / schemes; either because the old version of those classes just assumed DC records and thus needed to be changed to support other types; or to avoid copy-pasting a lot of code for each record type / scheme.
On a first glance, I can't see why those two maps in the two classes below use a String as key (localpart) rather than the whole Name. I'm surprised myself that I did that. The only reason I can imagine right now is that sometimes people might make requests without specifying the namespace, and I wanted to allow that lazy behaviour. Apart from that I see no reason why we can't change the use of String by the use of Name in those maps.
Kind Regards
Niels
On 25/02/14 15:02, Andrea Aime wrote:
Hi,
today I was trying to port over an ebRIM record implementation we made some time
ago and, besides the several difficulties caused by the many changes to
the base classes I've found that the core CSW classes regressed and cannot
tell apart anymore two record types with the same name, but different namespace
(we have a custom ebRim record that uses a different namespace and uses extensions, in parallel
with the standard one):
http://jira.codehaus.org/browse/GEOS-6368
This is due to the extensive usage of getLocalName() in the current implementation,
for example:
https://github.com/geoserver/geoserver/blob/2.4.x/src/extension/csw/api/src/main/java/org/geoserver/csw/store/AbstractCatalogStore.java#L49
https://github.com/geoserver/geoserver/blob/2.4.x/src/extension/csw/api/src/main/java/org/geoserver/csw/store/CatalogStoreCapabilities.java
Niels, I'm surprised that a developer with an app-schema background would go there,
so I guess there are good reasons for this change, and I'm just not seeing them?
Regardless, this is a regression and needs to be fixed, but I would like to understand what
challenges got us here to avoid exchanging one regression with another
Cheers
Andrea
--
== Our support, Your Success! Visit http://opensdi.geo-solutions.it for more information ==
Ing. Andrea Aime
@geowolf
Technical Lead
GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549
http://www.geo-solutions.it
http://twitter.com/geosolutions_it
-------------------------------------------------------
On Tue, Feb 25, 2014 at 3:33 PM, Niels Charlier <niels@anonymised.com> wrote:
Hello Andrea,
All the changes that were made to those base classes were done to make
those classes generic for different record types / schemes; either because
the old version of those classes just assumed DC records and thus needed to
be changed to support other types; or to avoid copy-pasting a lot of code
for each record type / scheme.
Hem... I've implemented functioning support for ebRIM records based on
those classes (that is, extending them, no code duplication), so they were
definitely not hard-coded to csw:Record (nor assuming it), although I
understand that ISO support presents different challenges than ebRIM and
thus likely required modifications.
On a first glance, I can't see why those two maps in the two classes below
use a String as key (localpart) rather than the whole Name. I'm surprised
myself that I did that. The only reason I can imagine right now is that
sometimes people might make requests without specifying the namespace, and
I wanted to allow that lazy behaviour. Apart from that I see no reason why
we can't change the use of String by the use of Name in those maps.
Cool, I guess I'll make the fix and try to add tests so that we don't
regress on this aspect in the future
Cheers
Andrea
--
== Our support, Your Success! Visit http://opensdi.geo-solutions.it for
more information ==
Ing. Andrea Aime
@geowolf
Technical Lead
GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549
http://www.geo-solutions.it
http://twitter.com/geosolutions_it
-------------------------------------------------------
On Tue, Feb 25, 2014 at 3:41 PM, Andrea Aime
<andrea.aime@anonymised.com>wrote:
On a first glance, I can't see why those two maps in the two classes below
use a String as key (localpart) rather than the whole Name. I'm surprised
myself that I did that. The only reason I can imagine right now is that
sometimes people might make requests without specifying the namespace, and
I wanted to allow that lazy behaviour. Apart from that I see no reason why
we can't change the use of String by the use of Name in those maps.
Cool, I guess I'll make the fix and try to add tests so that we don't
regress on this aspect in the future
Pull request here:
https://github.com/geoserver/geoserver/pull/506
I believe the original code was using the local part because the tests for
the simple store were never specifying the
namespace. I've made both work by rolling a Name -> RecordDescriptor map
that has some leniency, and if a full
name match against a key that has no namespace fails, attempts also a match
against the local part only
Cheers
Andrea
--
== Our support, Your Success! Visit http://opensdi.geo-solutions.it for
more information ==
Ing. Andrea Aime
@geowolf
Technical Lead
GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549
http://www.geo-solutions.it
http://twitter.com/geosolutions_it
-------------------------------------------------------
Looks good! Indeed, that must have been the reason I did it. This is a good solution! Regards Niels
···
On 28/02/14 11:38, Andrea Aime wrote:
On Tue, Feb 25, 2014 at 3:41 PM, Andrea Aime <andrea.aime@anonymised.com> wrote:
On a first glance, I can’t see why those two maps in the two classes below use a String as key (localpart) rather than the whole Name. I’m surprised myself that I did that. The only reason I can imagine right now is that sometimes people might make requests without specifying the namespace, and I wanted to allow that lazy behaviour. Apart from that I see no reason why we can’t change the use of String by the use of Name in those maps.
Cool, I guess I’ll make the fix and try to add tests so that we don’t regress on this aspect in the future
Pull request here:
https://github.com/geoserver/geoserver/pull/506
I believe the original code was using the local part because the tests for the simple store were never specifying the
namespace. I’ve made both work by rolling a Name → RecordDescriptor map that has some leniency, and if a full
name match against a key that has no namespace fails, attempts also a match against the local part only
Cheers
Andrea