[Geoserver-devel] Has the WFS GetFeature behavior changed?

Hi,
quick question... I've noticed that on trunk doing a query
with a limited parameter set will return not only those
requested, but also every parameter that's marked as required
in the feature type.

In my case, the geometry is marked as required (data is coming
from postgis) and as a result, I can't get the geometry out
of the GetFeature results.

I understand the principle (generating a document that's still
valid according to the feature type schema) but I find this
annoying nevertheless.

Are we bound to this behavior by WFS standard, or it's just a matter
of generating an alternate schema so that the returned document
validates anyways?
Cheers
Andrea

I'm pretty sure we're bound by the WFS standard on this. From the spec:

'The response to a GetFeature request must be valid according to the structure described by the XML Schema description of the feature type. Thus the WFS must report all the mandatory properties of each feature, as well any properties requested through the <wfs:PropertyName> element. In the event that a WFS encounters a query that does not select all mandatory properties of a feature, the WFS will internally augment the property name list to include all mandatory property names. A WFS client must thus be prepared to deal with a situation where it receives more property values than it requests through <wfs:PropertyName> elements.'

Can you change your data so the geometry isn't required?

Chris

Andrea Aime wrote:

Hi,
quick question... I've noticed that on trunk doing a query
with a limited parameter set will return not only those
requested, but also every parameter that's marked as required
in the feature type.

In my case, the geometry is marked as required (data is coming
from postgis) and as a result, I can't get the geometry out
of the GetFeature results.

I understand the principle (generating a document that's still
valid according to the feature type schema) but I find this
annoying nevertheless.

Are we bound to this behavior by WFS standard, or it's just a matter
of generating an alternate schema so that the returned document
validates anyways?
Cheers
Andrea

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

!DSPAM:1003,45f05b13126951665516417!

--
Chris Holmes
The Open Planning Project
http://topp.openplans.org

The behavior has not changed; required fields have always had a enforced include
inflicted on them. Enforcing this limitation is one of the reasons GeoServer has its own
feature source wrapper (rather then use GeoTools DataStore.getView( Query ) )

You can define an additional opperation (that generates a lax schema) and return a
feature results that uses this XSD. To remain conformant we would need to make a
seperate GetFeatureResults opperation (or use vendor specific parameters?)

Jody

Andrea Aime wrote:

Hi,
quick question... I've noticed that on trunk doing a query
with a limited parameter set will return not only those
requested, but also every parameter that's marked as required
in the feature type.

In my case, the geometry is marked as required (data is coming
from postgis) and as a result, I can't get the geometry out
of the GetFeature results.

I understand the principle (generating a document that's still
valid according to the feature type schema) but I find this
annoying nevertheless.

Are we bound to this behavior by WFS standard, or it's just a matter
of generating an alternate schema so that the returned document
validates anyways?
Cheers
Andrea

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
  

Chris Holmes ha scritto:

I'm pretty sure we're bound by the WFS standard on this. From the spec:

'The response to a GetFeature request must be valid according to the structure described by the XML Schema description of the feature type. Thus the WFS must report all the mandatory properties of each feature, as well any properties requested through the <wfs:PropertyName> element. In the event that a WFS encounters a query that does not select all mandatory properties of a feature, the WFS will internally augment the property name list to include all mandatory property names. A WFS client must thus be prepared to deal with a situation where it receives more property values than it requests through <wfs:PropertyName> elements.'

Thanks for the clarification (lazy lazy, I should have read the wfs
standard)

Can you change your data so the geometry isn't required?

Yeah, sure I can, I'll have to make the bbox in changeset nillable.
Bye
Andrea

Andrea Aime ha scritto:

Chris Holmes ha scritto:

an it requests through <wfs:PropertyName> elements.'

Can you change your data so the geometry isn't required?

Yeah, sure I can, I'll have to make the bbox in changeset nillable.

Well, it turns out the bbox is nullable... a describe feature
states the attribute is nillable and minoccurs is 0,
and yet when executing a GetFeature request the debugger
shows the AttributeTypeInfo for BBOX has minoccurs = 1....
crazy!

Oh well, enough for today, I'll discover what the heck
is going on Monday.

Cheers
Andrea

Chris Holmes ha scritto:

I'm pretty sure we're bound by the WFS standard on this. From the spec:

'The response to a GetFeature request must be valid according to the structure described by the XML Schema description of the feature type. Thus the WFS must report all the mandatory properties of each feature, as well any properties requested through the <wfs:PropertyName> element. In the event that a WFS encounters a query that does not select all mandatory properties of a feature, the WFS will internally augment the property name list to include all mandatory property names. A WFS client must thus be prepared to deal with a situation where it receives more property values than it requests through <wfs:PropertyName> elements.'

Can you change your data so the geometry isn't required?

Ouff, spent more than two hours on this, but found what changed.
The difference is made by a little change Justin committed one month ago, revision 6125, changelog "formatting", that did not contain only
formatting, on DataTransferObjectFactory.create(schemaBase, attributeType).

Yet, the real problem is not in Justin change, which really seems
the right thing to do, but in the overall way attribute type minOccurs
is handled, which is really a mess.

Let me summarize what is going on.
(Warning, long horror story ahead, don't let children come near the screen).
The tale begins in the feature type creation. All data stores do use
DefaultAttributeTypeFactory to build attribute types. On 2.3.x, that class always generates attributes with 1:1 min max occurrences.
On trunk, someone exposed a method that allows for creating attribute types with a custom min/max occurrences, but it's used only
by JDBC data stores for non geometric attributes (JDBC1DataStore.buildAttributeType).

So, long story short, every single datastore will generate attribute
types stating that the geometry is required. This does not seem right
to me, what do you think? I understand that for shapefiles, but it
seems to be wrong for JDBC based sources, or for the property data store (I guess).

Enter Geoserver and its DTO stuff. AttributeTypeInfo is a sort of
wrapper around the actual attribute type, and its minimum
occurrences is determined by something other than the actual
attribute type in 1.5.x.
DataTransferObject.create(schemaBase, AttributeType) contains:
dto.setMinOccurs(isManditory(schemaBase, attributeType.getName()) ? 1 : 0);

schemaBase is usually "AbstractFeatureType", that does not contain any
attribute, as a result, every single attribute in geoserver gets configured as optional, no matter what the feature type says.
This is obviously wrong for shapefiles geometries imho, at least
on the write perspective, thought if we change

In trunk, the story is a little different, because the code above says:
if (isManditory(schemaBase, attributeType.getName()) || (attributeType.getMinOccurs() > 0)) {
             dto.setMinOccurs(1);
         } else {
             dto.setMinOccurs(0);
         }

I guess Justin had to change this to make WFS 1.1 cite happy.

Now, I'm going to fix the postgis data store so that it takes into
account attribute nullability (Justin, I'll wait for your get go
to commit the patch), but it seems to me we have to fix somewhat
the Geosever behaviour too...

Cheers
Andrea

So, long story short, every single datastore will generate attribute
types stating that the geometry is required. This does not seem right
to me, what do you think? I understand that for shapefiles, but it
seems to be wrong for JDBC based sources, or for the property data store (I guess).

You are right, this is wrong!

Objects may have 0, 1 or many geometries. One of the compelling reasons (coming up) for using geoserver is that it can be a general purpose API to data, allowing spatial and non-spatial views into the same data with a single insyall, instead of one of the fragile database serialisation mechanisms, that bind people to both your technology stack and the the persistence schema.

Now, I'm going to fix the postgis data store so that it takes into
account attribute nullability (Justin, I'll wait for your get go
to commit the patch), but it seems to me we have to fix somewhat
the Geosever behaviour too...

Possibly Gabriel may have encountered this working on geoserver trunk to expose ISO features.

Rob

Cheers
Andrea

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
  

Hi Andrea,

I think there may be a minor issue with the patch. Is there a patch to
look at? I am not sure of exactly the change you are proposing but i
think i ran up against this same issue. If you look at line 1269 on
JDBC1DataStore there is a comment explaining the problem. Basically it
boils down to how some of the feature readers and how they copy
features. Basically some of them just create a null array of attributes
and then set the objects one by one. However if the feature type says
that an attribute cannot be null that code will fail. So the workaround
was to setting minoccurs to > 0 if the attribute was indeed nillable.

So I think the fix will be cool, but like you say I think we will have
to fix code in other places too in both geotools and geoserver.

-Justin

Andrea Aime wrote:

Chris Holmes ha scritto:

I'm pretty sure we're bound by the WFS standard on this. From the spec:

'The response to a GetFeature request must be valid according to the
structure described by the XML Schema description of the feature type.
Thus the WFS must report all the mandatory properties of each feature,
as well any properties requested through the <wfs:PropertyName>
element. In the event that a WFS encounters a query that does not
select all mandatory properties of a feature, the WFS will internally
augment the property name list to include all mandatory property
names. A WFS client must thus be prepared to deal with a situation
where it receives more property values than it requests through
<wfs:PropertyName> elements.'

Can you change your data so the geometry isn't required?

Ouff, spent more than two hours on this, but found what changed.
The difference is made by a little change Justin committed one month
ago, revision 6125, changelog "formatting", that did not contain only
formatting, on DataTransferObjectFactory.create(schemaBase, attributeType).

Yet, the real problem is not in Justin change, which really seems
the right thing to do, but in the overall way attribute type minOccurs
is handled, which is really a mess.

Let me summarize what is going on.
(Warning, long horror story ahead, don't let children come near the
screen).
The tale begins in the feature type creation. All data stores do use
DefaultAttributeTypeFactory to build attribute types. On 2.3.x, that
class always generates attributes with 1:1 min max occurrences.
On trunk, someone exposed a method that allows for creating attribute
types with a custom min/max occurrences, but it's used only
by JDBC data stores for non geometric attributes
(JDBC1DataStore.buildAttributeType).

So, long story short, every single datastore will generate attribute
types stating that the geometry is required. This does not seem right
to me, what do you think? I understand that for shapefiles, but it
seems to be wrong for JDBC based sources, or for the property data store
(I guess).

Enter Geoserver and its DTO stuff. AttributeTypeInfo is a sort of
wrapper around the actual attribute type, and its minimum
occurrences is determined by something other than the actual
attribute type in 1.5.x.
DataTransferObject.create(schemaBase, AttributeType) contains:
dto.setMinOccurs(isManditory(schemaBase, attributeType.getName()) ? 1 : 0);

schemaBase is usually "AbstractFeatureType", that does not contain any
attribute, as a result, every single attribute in geoserver gets
configured as optional, no matter what the feature type says.
This is obviously wrong for shapefiles geometries imho, at least
on the write perspective, thought if we change

In trunk, the story is a little different, because the code above says:
if (isManditory(schemaBase, attributeType.getName()) ||
(attributeType.getMinOccurs() > 0)) {
            dto.setMinOccurs(1);
        } else {
            dto.setMinOccurs(0);
        }

I guess Justin had to change this to make WFS 1.1 cite happy.

Now, I'm going to fix the postgis data store so that it takes into
account attribute nullability (Justin, I'll wait for your get go
to commit the patch), but it seems to me we have to fix somewhat
the Geosever behaviour too...

Cheers
Andrea

!DSPAM:1004,45f5257095731194215290!

--
Justin Deoliveira
The Open Planning Project
http://topp.openplans.org

Justin Deoliveira ha scritto:

Hi Andrea,

I think there may be a minor issue with the patch. Is there a patch to
look at? I am not sure of exactly the change you are proposing but i
think i ran up against this same issue. If you look at line 1269 on
JDBC1DataStore there is a comment explaining the problem. Basically it
boils down to how some of the feature readers and how they copy
features. Basically some of them just create a null array of attributes
and then set the objects one by one. However if the feature type says
that an attribute cannot be null that code will fail. So the workaround
was to setting minoccurs to > 0 if the attribute was indeed nillable.

So I think the fix will be cool, but like you say I think we will have
to fix code in other places too in both geotools and geoserver.

Yeah, the patch follows the directions in that comment (I hope).
Here it is.

----------------------------------------------------------------------------------------

Index: C:/progetti/geotools/trunk/modules/plugin/postgis/src/main/java/org/geotools/data/postgis/PostgisDataStore.java

--- C:/progetti/geotools/trunk/modules/plugin/postgis/src/main/java/org/geotools/data/postgis/PostgisDataStore.java (revision 24721)
+++ C:/progetti/geotools/trunk/modules/plugin/postgis/src/main/java/org/geotools/data/postgis/PostgisDataStore.java (working copy)
@@ -943,13 +943,31 @@
              final int TABLE_NAME = 3;
              final int COLUMN_NAME = 4;
              final int TYPE_NAME = 6;
+ final int NULLABLE = 11;
              String typeName = metadataRs.getString(TYPE_NAME);
-
+
              if (typeName.equals("geometry")) {
                  String tableName = metadataRs.getString(TABLE_NAME);
                  String columnName = metadataRs.getString(COLUMN_NAME);
+
+ // check for nullability
+ int nullCode = metadataRs.getInt( NULLABLE );
+ boolean nillable = true;
+ switch( nullCode ) {
+ case DatabaseMetaData.columnNoNulls:
+ nillable = false;
+ break;
+
+ case DatabaseMetaData.columnNullable:
+ nillable = true;
+ break;
+
+ case DatabaseMetaData.columnNullableUnknown:
+ nillable = true;
+ break;
+ }

- return getGeometryAttribute(tableName, columnName);
+ return getGeometryAttribute(tableName, columnName, nillable);
              } else {
                  return super.buildAttributeType(metadataRs);
              }
@@ -985,6 +1003,7 @@
       *
       * @param tableName The feature table name.
       * @param columnName The geometry column name.
+ * @param nillable
       *
       * @return Geometric attribute.
       *
@@ -995,7 +1014,7 @@
       * @task This should probably take a Transaction, so if things mess up then
       * we can rollback.
       */
- AttributeType getGeometryAttribute(String tableName, String columnName)
+ AttributeType getGeometryAttribute(String tableName, String columnName, boolean nillable)
          throws IOException {
          Connection dbConnection = null;
          Class type = null;
@@ -1095,8 +1114,10 @@
          } catch (FactoryException e) {
              crs = null;
          }
-
- return AttributeTypeFactory.newAttributeType(columnName, type, true, 0, null, crs);
+
+ int min = nillable ? 0 : 1;
+ return AttributeTypeFactory.newAttributeType(columnName, type, true, null, null, crs, min, 1 );
+// return AttributeTypeFactory.newAttributeType(columnName, type, true, 0, null, crs);
      }

      private PostgisAuthorityFactory getPostgisAuthorityFactory() {

----------------------------------------------------------------------------------------

Cheers
Andrea

Cool, patch looks great Andrea.

Andrea Aime wrote:

Justin Deoliveira ha scritto:

Hi Andrea,

I think there may be a minor issue with the patch. Is there a patch to
look at? I am not sure of exactly the change you are proposing but i
think i ran up against this same issue. If you look at line 1269 on
JDBC1DataStore there is a comment explaining the problem. Basically it
boils down to how some of the feature readers and how they copy
features. Basically some of them just create a null array of attributes
and then set the objects one by one. However if the feature type says
that an attribute cannot be null that code will fail. So the workaround
was to setting minoccurs to > 0 if the attribute was indeed nillable.

So I think the fix will be cool, but like you say I think we will have
to fix code in other places too in both geotools and geoserver.

Yeah, the patch follows the directions in that comment (I hope).
Here it is.

----------------------------------------------------------------------------------------

Index:
C:/progetti/geotools/trunk/modules/plugin/postgis/src/main/java/org/geotools/data/postgis/PostgisDataStore.java

===================================================================
---
C:/progetti/geotools/trunk/modules/plugin/postgis/src/main/java/org/geotools/data/postgis/PostgisDataStore.java
(revision 24721)
+++
C:/progetti/geotools/trunk/modules/plugin/postgis/src/main/java/org/geotools/data/postgis/PostgisDataStore.java
(working copy)
@@ -943,13 +943,31 @@
             final int TABLE_NAME = 3;
             final int COLUMN_NAME = 4;
             final int TYPE_NAME = 6;
+ final int NULLABLE = 11;
             String typeName = metadataRs.getString(TYPE_NAME);
-
+
             if (typeName.equals("geometry")) {
                 String tableName = metadataRs.getString(TABLE_NAME);
                 String columnName = metadataRs.getString(COLUMN_NAME);
+
+ // check for nullability
+ int nullCode = metadataRs.getInt( NULLABLE );
+ boolean nillable = true;
+ switch( nullCode ) {
+ case DatabaseMetaData.columnNoNulls:
+ nillable = false;
+ break;
+
+ case DatabaseMetaData.columnNullable:
+ nillable = true;
+ break;
+
+ case DatabaseMetaData.columnNullableUnknown:
+ nillable = true;
+ break;
+ }

- return getGeometryAttribute(tableName, columnName);
+ return getGeometryAttribute(tableName, columnName,
nillable);
             } else {
                 return super.buildAttributeType(metadataRs);
             }
@@ -985,6 +1003,7 @@
      *
      * @param tableName The feature table name.
      * @param columnName The geometry column name.
+ * @param nillable
      *
      * @return Geometric attribute.
      *
@@ -995,7 +1014,7 @@
      * @task This should probably take a Transaction, so if things mess
up then
      * we can rollback.
      */
- AttributeType getGeometryAttribute(String tableName, String
columnName)
+ AttributeType getGeometryAttribute(String tableName, String
columnName, boolean nillable)
         throws IOException {
         Connection dbConnection = null;
         Class type = null;
@@ -1095,8 +1114,10 @@
         } catch (FactoryException e) {
             crs = null;
         }
-
- return AttributeTypeFactory.newAttributeType(columnName, type,
true, 0, null, crs);
+
+ int min = nillable ? 0 : 1;
+ return AttributeTypeFactory.newAttributeType(columnName, type,
true, null, null, crs, min, 1 );
+// return AttributeTypeFactory.newAttributeType(columnName,
type, true, 0, null, crs);
     }

     private PostgisAuthorityFactory getPostgisAuthorityFactory() {

----------------------------------------------------------------------------------------

Cheers
Andrea

!DSPAM:1004,45f56138128181775926497!

--
Justin Deoliveira
The Open Planning Project
http://topp.openplans.org

Justin Deoliveira ha scritto:

Cool, patch looks great Andrea.

The trouble is, what about the other data stores?
Shapefile won't work as in 1.5.x in Geoserver 1.6.0...
Plus there is oracle, db2, sde, and so on...

Was thinking about it, the right thing to do seems
to make the attribute factory take care of this, since
it receives the "nillable" attribute, use it to determine
min occurrences without having to patch each datastore.

Cheers
Andrea

Let's leave attribute factory stupid ... we need to switch over to using SimpleTypeBuilder anyways ... put the smarts there. This bug gives a good reason to go in there and make the change. (well other then the fact it is needed for 2.4)

Cheers,
Jody

Justin Deoliveira ha scritto:
  

Cool, patch looks great Andrea.
    

The trouble is, what about the other data stores?
Shapefile won't work as in 1.5.x in Geoserver 1.6.0...
Plus there is oracle, db2, sde, and so on...

Was thinking about it, the right thing to do seems
to make the attribute factory take care of this, since
it receives the "nillable" attribute, use it to determine
min occurrences without having to patch each datastore.

Cheers
Andrea

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
  

Jody Garnett ha scritto:

Let's leave attribute factory stupid ... we need to switch over to using SimpleTypeBuilder anyways ... put the smarts there. This bug gives a good reason to go in there and make the change. (well other then the fact it is needed for 2.4)

Jody, I'm supposed to work on versioned datastore, not on switching
over to simple type builder. To make the switch I would have to work
on all jdbc data store, test them all... not something I can do, it would take at last a full day to make it seriously (and I don't have
MySql around).

I insist, is there any good reason for not putting the logic
in default attribute factory? That would be a few lines of code only.

Cheers
Andrea

The reason is the next factory implementation will break your assumption (about how min/max relates to nullability).

Last time we had this kind of conversation I spent a couple days fixing query, is this the same kind of deal?

I don't want to see us doing isolated fixes - I want to see the class responsibilities clearly defined and tested for.
If you find the codebase too large then don't update the unsupported jdbc datastores, or leave the hacks in GeoServer until
someone does have the time to fix GeoTools.

I understand that this SimpleFeatureBuilder change is a sore point that everyone wishes another person would do. Just like with
the providence review this is thankless work - and the reason we have a PMC. Your bug may be the motivation needed to call
together a work part on this topic.

Cheers,
Jody

Jody Garnett ha scritto:

Let's leave attribute factory stupid ... we need to switch over to using SimpleTypeBuilder anyways ... put the smarts there. This bug gives a good reason to go in there and make the change. (well other then the fact it is needed for 2.4)

Jody, I'm supposed to work on versioned datastore, not on switching
over to simple type builder. To make the switch I would have to work
on all jdbc data store, test them all... not something I can do, it would take at last a full day to make it seriously (and I don't have
MySql around).

I insist, is there any good reason for not putting the logic
in default attribute factory? That would be a few lines of code only.

Cheers
Andrea

Yup, yet another place where datastores act differently. Doing it at the
factory is a possibility, and yeah it would be something global which is
nice. However the datastores would still have to do the work to actually
figure out if the attribute is nillable.

However if speaking in strictly xml terms, i dont think that nillable
implies that minOccurs = 0 as Xml has the notion of an explicit nil.
However the wfs spec specifies that if an attribute instance is null you
can just omit it, so the same behaviour.

Again keep in mind that this is something that is undoubetly going to
change with the new feature model, so gabriels feedback on how this is
handled will be valuable.

Andrea Aime wrote:

Justin Deoliveira ha scritto:

Cool, patch looks great Andrea.

The trouble is, what about the other data stores?
Shapefile won't work as in 1.5.x in Geoserver 1.6.0...
Plus there is oracle, db2, sde, and so on...

Was thinking about it, the right thing to do seems
to make the attribute factory take care of this, since
it receives the "nillable" attribute, use it to determine
min occurrences without having to patch each datastore.

Cheers
Andrea

!DSPAM:1004,45f563b9131447785049143!

--
Justin Deoliveira
The Open Planning Project
http://topp.openplans.org

Its a tough call. Based on correctness jody, I agree with you. But with
the bandaged feature model and datastores we have how can people like
Andrea who have deadlines hold out. Especially since the new feature
model work is still just a branch with no plan to come home. I myself
want to keep weary of the new model... but I also understand that is
still a far time off. So until we have a plan and funding, we are
probably going to have to put up with little fixes such as these and
doing things the "wrong way".

My 2c

Jody Garnett wrote:

The reason is the next factory implementation will break your assumption
(about how min/max relates to nullability).

Last time we had this kind of conversation I spent a couple days fixing
query, is this the same kind of deal?

I don't want to see us doing isolated fixes - I want to see the class
responsibilities clearly defined and tested for.
If you find the codebase too large then don't update the unsupported
jdbc datastores, or leave the hacks in GeoServer until
someone does have the time to fix GeoTools.

I understand that this SimpleFeatureBuilder change is a sore point that
everyone wishes another person would do. Just like with
the providence review this is thankless work - and the reason we have a
PMC. Your bug may be the motivation needed to call
together a work part on this topic.

Cheers,
Jody

Jody Garnett ha scritto:

Let's leave attribute factory stupid ... we need to switch over to
using SimpleTypeBuilder anyways ... put the smarts there. This bug
gives a good reason to go in there and make the change. (well other
then the fact it is needed for 2.4)

Jody, I'm supposed to work on versioned datastore, not on switching
over to simple type builder. To make the switch I would have to work
on all jdbc data store, test them all... not something I can do, it
would take at last a full day to make it seriously (and I don't have
MySql around).

I insist, is there any good reason for not putting the logic
in default attribute factory? That would be a few lines of code only.

Cheers
Andrea

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

!DSPAM:1004,45f5a14c194091365099012!

--
Justin Deoliveira
The Open Planning Project
http://topp.openplans.org

Thanks Justin:

We covered most of this in the IRC meeting today. I do understand that deadlines must be met (we
don't want Andrea dead after all).

Being accountable for patches (and keeping the big picture in mind) is the other half of keeping
trunk stable - and makes me glad we are all working on trunk these days.

Jody

Its a tough call. Based on correctness jody, I agree with you. But with
the bandaged feature model and datastores we have how can people like
Andrea who have deadlines hold out. Especially since the new feature
model work is still just a branch with no plan to come home. I myself
want to keep weary of the new model... but I also understand that is
still a far time off. So until we have a plan and funding, we are
probably going to have to put up with little fixes such as these and
doing things the "wrong way".

My 2c

Jody Garnett wrote:
  

The reason is the next factory implementation will break your assumption (about how min/max relates to nullability).

Last time we had this kind of conversation I spent a couple days fixing query, is this the same kind of deal?

I don't want to see us doing isolated fixes - I want to see the class responsibilities clearly defined and tested for.
If you find the codebase too large then don't update the unsupported jdbc datastores, or leave the hacks in GeoServer until
someone does have the time to fix GeoTools.

I understand that this SimpleFeatureBuilder change is a sore point that everyone wishes another person would do. Just like with
the providence review this is thankless work - and the reason we have a PMC. Your bug may be the motivation needed to call
together a work part on this topic.

Cheers,
Jody

Jody Garnett ha scritto:
      

Let's leave attribute factory stupid ... we need to switch over to using SimpleTypeBuilder anyways ... put the smarts there. This bug gives a good reason to go in there and make the change. (well other then the fact it is needed for 2.4)
        

Jody, I'm supposed to work on versioned datastore, not on switching
over to simple type builder. To make the switch I would have to work
on all jdbc data store, test them all... not something I can do, it would take at last a full day to make it seriously (and I don't have
MySql around).

I insist, is there any good reason for not putting the logic
in default attribute factory? That would be a few lines of code only.

Cheers
Andrea
      

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

!DSPAM:1004,45f5a14c194091365099012!

Hi Andrea - thanks for the patch.

I found a couple spots you missed (where minOccurs was still 1); and will commit soon after the tests pass.

For the future changes on trunk need to be punted out to javadocs; I updated them for you this time as part
of my review.

Cheers,
Jody

Thanks Justin:

We covered most of this in the IRC meeting today. I do understand that deadlines must be met (we
don't want Andrea dead after all).

Being accountable for patches (and keeping the big picture in mind) is the other half of keeping
trunk stable - and makes me glad we are all working on trunk these days.

Jody

Its a tough call. Based on correctness jody, I agree with you. But with
the bandaged feature model and datastores we have how can people like
Andrea who have deadlines hold out. Especially since the new feature
model work is still just a branch with no plan to come home. I myself
want to keep weary of the new model... but I also understand that is
still a far time off. So until we have a plan and funding, we are
probably going to have to put up with little fixes such as these and
doing things the "wrong way".

My 2c

Jody Garnett wrote:
  

The reason is the next factory implementation will break your assumption (about how min/max relates to nullability).

Last time we had this kind of conversation I spent a couple days fixing query, is this the same kind of deal?

I don't want to see us doing isolated fixes - I want to see the class responsibilities clearly defined and tested for.
If you find the codebase too large then don't update the unsupported jdbc datastores, or leave the hacks in GeoServer until
someone does have the time to fix GeoTools.

I understand that this SimpleFeatureBuilder change is a sore point that everyone wishes another person would do. Just like with
the providence review this is thankless work - and the reason we have a PMC. Your bug may be the motivation needed to call
together a work part on this topic.

Cheers,
Jody

Jody Garnett ha scritto:
      

Let's leave attribute factory stupid ... we need to switch over to using SimpleTypeBuilder anyways ... put the smarts there. This bug gives a good reason to go in there and make the change. (well other then the fact it is needed for 2.4)
        

Jody, I'm supposed to work on versioned datastore, not on switching
over to simple type builder. To make the switch I would have to work
on all jdbc data store, test them all... not something I can do, it would take at last a full day to make it seriously (and I don't have
MySql around).

I insist, is there any good reason for not putting the logic
in default attribute factory? That would be a few lines of code only.

Cheers
Andrea
      

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

!DSPAM:1004,45f5a14c194091365099012!

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel