Hi all.
I’m looking over the findbugs reports for the main module (we ran into some snags integrating it into the nightly build but Justin is helping me investigate; in the meantime I am just looking over the unfiltered results locally.) There seem to be some flagged sections in the catalog. From initial review they look like real bugs, but in fixing them I have found that the tests appear to be buggy as well[1]. It would help if I had some specs on what guarantees the catalog is expected to enforce, but after looking at the code/javadocs and the developer’s manual I haven’t found it. If such a document exists, where can I find it? If not, how can we figure out what the expected validation rules are supposed to be? I could review the existing validations in CatalogImpl so we can start with a document describing what’s there right now.
[1]: For example, CatalogImplTest::testAddLayer tests adding a layer without a name, then adding a layer without a resource. It is apparently supposed to test the no-name validation by creating a layer and immediately saving it (ie, the layer is invalid in more ways than just lacking a name.) Then it adds a name and tests that layers without a resource should fail validation - but the operation to add the name is commented out, meaning that the no-name validation is just tested again.
–
David Winslow
OpenGeo - http://opengeo.org/
On Tue, Mar 13, 2012 at 10:40 PM, David Winslow <dwinslow@anonymised.com> wrote:
Hi all.
I'm looking over the findbugs reports for the main module (we ran into some
snags integrating it into the nightly build but Justin is helping me
investigate; in the meantime I am just looking over the unfiltered results
locally.) There seem to be some flagged sections in the catalog. From
initial review they look like real bugs, but in fixing them I have found
that the tests appear to be buggy as well[1]. It would help if I had some
specs on what guarantees the catalog is expected to enforce, but after
looking at the code/javadocs and the developer's manual I haven't found it.
If such a document exists, where can I find it? If not, how can we figure
out what the expected validation rules are supposed to be? I could review
the existing validations in CatalogImpl so we can start with a document
describing what's there right now.
As far as I know, there is no such a thing as a spec.
Actually this makes me think about the current commit requirements
(patches + tests)
and the fact that I've been meaning to discuss raising them to also have user
documentation and or tech documentation in the mix.
Quite a bit hesitant on the topic though, besides the core
contributors it's already
hard to see a patch proposed with a proper test, asking for docs too might
actually kill any attempt to contribute stuff outside of the core devs...
Cheers
Andrea
--
-------------------------------------------------------
Ing. Andrea Aime
GeoSolutions S.A.S.
Tech lead
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 962313
mob: +39 339 8844549
http://www.geo-solutions.it
http://geo-solutions.blogspot.com/
http://www.youtube.com/user/GeoSolutionsIT
http://www.linkedin.com/in/andreaaime
http://twitter.com/geowolf
-------------------------------------------------------
Well this type of documentation is different from user docs right… this is api documentation which is something we have never focused on. The geoserver “api” in general is something we have never really focused on and have very few guidelines for.
Anyways, indeed there are no specs about what validation occurs for catalog data objects. All the validation occurs in methods named validate() in CatalogImpl and the logic is pretty straight forward to follow.
This specific issue with layer names is most likely a side effect of the fact that we changed the implementation of LayerInfo to always have the same name as its underlying resource. Eventually we would like to break this and have a layer name be independent of its resource (part of the resource/publishing split idea) but for the time being it was thought it better to ensure the two are always in sync until we explicitly support that feature. Look at LayerInfoImpl.getName() to see what i mean.
On Wed, Mar 14, 2012 at 4:39 AM, Andrea Aime <andrea.aime@anonymised.com> wrote:
On Tue, Mar 13, 2012 at 10:40 PM, David Winslow <dwinslow@anonymised.com> wrote:
Hi all.
I’m looking over the findbugs reports for the main module (we ran into some
snags integrating it into the nightly build but Justin is helping me
investigate; in the meantime I am just looking over the unfiltered results
locally.) There seem to be some flagged sections in the catalog. From
initial review they look like real bugs, but in fixing them I have found
that the tests appear to be buggy as well[1]. It would help if I had some
specs on what guarantees the catalog is expected to enforce, but after
looking at the code/javadocs and the developer’s manual I haven’t found it.
If such a document exists, where can I find it? If not, how can we figure
out what the expected validation rules are supposed to be? I could review
the existing validations in CatalogImpl so we can start with a document
describing what’s there right now.
As far as I know, there is no such a thing as a spec.
Actually this makes me think about the current commit requirements
(patches + tests)
and the fact that I’ve been meaning to discuss raising them to also have user
documentation and or tech documentation in the mix.
Quite a bit hesitant on the topic though, besides the core
contributors it’s already
hard to see a patch proposed with a proper test, asking for docs too might
actually kill any attempt to contribute stuff outside of the core devs…
Cheers
Andrea
–
Ing. Andrea Aime
GeoSolutions S.A.S.
Tech lead
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 962313
mob: +39 339 8844549
http://www.geo-solutions.it
http://geo-solutions.blogspot.com/
http://www.youtube.com/user/GeoSolutionsIT
http://www.linkedin.com/in/andreaaime
http://twitter.com/geowolf
Virtualization & Cloud Management Using Capacity Planning
Cloud computing makes use of virtualization - but cloud computing
also focuses on allowing computing to be delivered as a service.
http://www.accelacomm.com/jaw/sfnl/114/51521223/
Geoserver-devel mailing list
Geoserver-devel@anonymised.comsts.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.
I see, the layer name constraint is not really necessary because we enforce uniqueness among layer resources. However, the code to enforce resource uniquness is what triggered findbugs in the first place so I think there are still some changes needed here. I created a patch, attached to http://jira.codehaus.org/browse/GEOS-5001
–
David Winslow
OpenGeo - http://opengeo.org/
On Wed, Mar 14, 2012 at 9:03 AM, Justin Deoliveira <jdeolive@anonymised.com…1501…> wrote:
Well this type of documentation is different from user docs right… this is api documentation which is something we have never focused on. The geoserver “api” in general is something we have never really focused on and have very few guidelines for.
Anyways, indeed there are no specs about what validation occurs for catalog data objects. All the validation occurs in methods named validate() in CatalogImpl and the logic is pretty straight forward to follow.
This specific issue with layer names is most likely a side effect of the fact that we changed the implementation of LayerInfo to always have the same name as its underlying resource. Eventually we would like to break this and have a layer name be independent of its resource (part of the resource/publishing split idea) but for the time being it was thought it better to ensure the two are always in sync until we explicitly support that feature. Look at LayerInfoImpl.getName() to see what i mean.
On Wed, Mar 14, 2012 at 4:39 AM, Andrea Aime <andrea.aime@anonymised.com> wrote:
On Tue, Mar 13, 2012 at 10:40 PM, David Winslow <dwinslow@anonymised.com> wrote:
Hi all.
I’m looking over the findbugs reports for the main module (we ran into some
snags integrating it into the nightly build but Justin is helping me
investigate; in the meantime I am just looking over the unfiltered results
locally.) There seem to be some flagged sections in the catalog. From
initial review they look like real bugs, but in fixing them I have found
that the tests appear to be buggy as well[1]. It would help if I had some
specs on what guarantees the catalog is expected to enforce, but after
looking at the code/javadocs and the developer’s manual I haven’t found it.
If such a document exists, where can I find it? If not, how can we figure
out what the expected validation rules are supposed to be? I could review
the existing validations in CatalogImpl so we can start with a document
describing what’s there right now.
As far as I know, there is no such a thing as a spec.
Actually this makes me think about the current commit requirements
(patches + tests)
and the fact that I’ve been meaning to discuss raising them to also have user
documentation and or tech documentation in the mix.
Quite a bit hesitant on the topic though, besides the core
contributors it’s already
hard to see a patch proposed with a proper test, asking for docs too might
actually kill any attempt to contribute stuff outside of the core devs…
Cheers
Andrea
–
Ing. Andrea Aime
GeoSolutions S.A.S.
Tech lead
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 962313
mob: +39 339 8844549
http://www.geo-solutions.it
http://geo-solutions.blogspot.com/
http://www.youtube.com/user/GeoSolutionsIT
http://www.linkedin.com/in/andreaaime
http://twitter.com/geowolf
Virtualization & Cloud Management Using Capacity Planning
Cloud computing makes use of virtualization - but cloud computing
also focuses on allowing computing to be delivered as a service.
http://www.accelacomm.com/jaw/sfnl/114/51521223/
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.