Re: [Geotools-devel] Validation again (for the last time? :-) )

[Jody G. <jod...@gm...>]

Andrea Aime wrote:
> so yesterday I was looking into how making validation
> optional into the current feature model.
>   
yeah!
> First off, we have this idea that features might be
> allowed to hold invalid attributes, and that we can
> use a isValid() method to check whether they
> are actually valid or not.
>   
understood.
> Implementation wise, this triggers the first issue:
> that method does not tell what is wrong, but only
> that something is wrong.
> A different approach could be to have a
> validate() throws IllegalAttributeException
> and have the exception report exactly what's wrong
> with the feature.
>   
Difficulty is that this either needs to be a structures exception 
message (indicating say the xpath to the problem and kind of problem) or 
we should
provides a utility method to do this kind of check.
> Moreover, even having that method, someone might want to have continuous checks on the feature model
> like we do now, that is, have the builders and the model throw exceptions when an invalid
> attribute value is provided.
>
> Do we really want to do that? Or we just tell the world that validation checks must be explicit, that is, never ask the model to do them automatically?
>   
I had pictured a useful compromise;
- have validation off by default
- have always-on-validation as a "strict" option to help debug problems

Given the choice; validation off is the way to go.
> If on the contrary we believe it's important and we allow the user to customize that behaviour, Jody was suggesting to do so by providing two different feature factories,
> one that builds non validating features, the other that builds
> validating ones... which in the end would boil down to
> parametrize the SimpleFeatureImpl with a flag that enables
> validation when attributes are set.
> It would also require the builder static utility methods, which
> are very commonly used in the datastores, to accept a factory.
> To make it effective it would mean we'd need to add a new
> Query hint (the feature factory, which is not there) and use it
> in all data stores.
> Yet that would not be all, those methods are used in
> many many places (SimpleFeatureBuilder.build(...) is used in 179
> different places according to Eclipse), do we want add a factory
> param in each place? Just have a look in DataUtilities class
> to get an idea.
>   
It was these bigger picture questions that were being talked about as 
the builder was introduced. The SimpleFeatureBuilder.build method is 
always a short cut hack; mostly to enable the DataUtility methods to 
work ...
How about for these cases we consider "validation off" to be the rule 
rather than the exception.
> Given that most non datastore methods are transformation related ones, we could use the same factory that built the original features, adding a getFactory() method to the Property interface...
> thought of course that would be another geoapi change, would
> require even more changes around, well, you get the picture.
>
> It seems to me this kind of change would require at least a one
> day sprint... what do we do in the meantime?
>   

Backing up:
- Our goal was to make the FeatureTypes static utility method available 
for people to find; we have now done that correct?
- For detailed reporting on why something is invalid - a good idea - we 
should treat that as a separate problem; it is something people can use 
a utility method for. Once we understand the problem we can consider 
adding it to the API. Code first then API.
- We should only worry about the use of SimpleFeatureBuilder as a 
builder ie the static methods on SimpleFeatureBuilder and in 
DataUtilities are assumptions used throughout our library; they saved us 
time when making the migration. You should be able to inline 
SimpleFeatureBuilder.build() as needed...
- For the static methods we should make use of a system wide 
FetaureFactory - assuming validation off by default.

Aside: The idea of remembering the factory seems like a little bit of 
gain; while I had similar thoughts they come from a different problem 
When working with Shapefiles only a limited set of content is supported 
(max string length etc...). Having 
ShapefileDataStore.getFeatureTypeFactory() return a factory that knows 
about these limitations is a sensible way to go...


Jody