You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by Carsten Ziegeler <cz...@apache.org> on 2012/08/13 11:47:55 UTC
Re: Empty string property values
While looking into SLING-2534, it seems that (as Alex noted) our docs
say something different than the implementation.
So I tried to fix the implementation (whic is fairly easy), but then I
ran into failing junit tests - I think these tests have wrong
assumptions.
So after fixing these I run into failing integration tests, e.g.
SlingDefaultValuesTest#testDefaultBehaviour posts a new property with
an empty value and checks if the property does not exists afterwards.
Now, these tests can be fixed as well, but I'm wondering if that's a
good thing - it would be correct, but existing applications might rely
on the wrong behaviour?
On the other hand for rendering a missing value and an empty value
usually doesn't make a difference...while it might make a difference
for other apps
Carsten
2012/7/19 Alexander Klimetschek <ak...@adobe.com>:
> On 16.07.2012, at 17:07, Jeremy Booth wrote:
>> For our app we do need to
>> differentiate between provided empty and not provided.
>
> I can imagine that this makes sense sometimes (albeit I haven't come across this yet).
>
>> On 16 July 2012 15:42, Carsten Ziegeler <cz...@apache.org> wrote:
>>> the handling of the empty value happens in the Sling Post servlet -
>>> afaik this was a decision in the early days of the post servlet to
>>> treat an empty value like no value.
>
> Yes, that was always in the code (I remember refactoring that once for the @Patch feature). It's clearly commented, albeit the initial reasoning seems lost:
>
> // if no value is present or a single empty string is given,
> // just remove the existing property (if any)
> removeProperty(parent, prop);
>
> However, the sling post servlet docs say something different:
>
> http://sling.apache.org/site/manipulating-content-the-slingpostservlet-servletspost.html#ManipulatingContent-TheSlingPostServlet%28servlets.post%29-%7B%7B@IgnoreBlanks%7D%7D
>
> The 2nd example in that section:
>
> <form method="POST" action="/content/page/first" enctype="multipart/form-data">
> <input type="hidden" name="stringProperty@TypeHint" value="String"/>
> <input type="text" name="stringProperty" value=""/>
> </form>
>
> This will _not_ result in an empty property, but in no property at all (just tested it).
>
> Without knowing the original reasoning for "" == null, I would say the sling post servlet documentation is king and this is a bug :-)
>
> But one needs to be careful here, as there are so many options involved: single- vs. multi-value properties, @IgnoreBlanks, @DefaultValue = :null, @DefaultValue = :ignore ... (see also RequestPropertyTest).
>
> Cheers,
> Alex
>
--
Carsten Ziegeler
cziegeler@apache.org
Re: Empty string property values
Posted by Alexander Klimetschek <ak...@adobe.com>.
On 13.08.2012, at 13:47, Bertrand Delacretaz <bd...@apache.org> wrote:
> I don't think IgnoreBlanks can be used to remove properties, IIUC it
> just causes request parameters with empty values to be ignored. So IMO
> there would be no way to remove a property if we changed the current
> behavior.
property@Delete = true ?
Cheers,
Alex
Re: Empty string property values
Posted by Carsten Ziegeler <cz...@apache.org>.
2012/8/13 Bertrand Delacretaz <bd...@apache.org>:
> On Mon, Aug 13, 2012 at 2:14 PM, Carsten Ziegeler <cz...@apache.org> wrote:
>> 2012/8/13 Bertrand Delacretaz <bd...@apache.org>:
>
>>> ...The first SLING-1412 commit is svn revision 916419, and the
>>> SlingDefaultValuesTest#testDefaultBehaviour test hasn't changed since
>>> rev 656302, so I think the "remove property on empty value" behavior
>>> has been there from rev 656302 or even earlier.
>>
>> Yes, I have the same feeling, but then why has SLING-1412 filed? :)
>
> IIUC the use case for that is a request with the following parameters
>
> /stringProperty@TypeHint=String[]
> ./stringProperty@IgnoreBlanks=true
> ./stringProperty=foo
> ./stringProperty=bar
> ./stringProperty=
>
> which should set only two values in the stringProperty multi-value
> field, instead of three without @IgnoreBlanks.
>
> If I'm right we could probably restrict IgnoreBlanks to multi-value
> properties, if that helps clarifying things.
>
>> ...the current code only implements some strange behaviour for
>> @IgnoreBlanks which in one way or the other has to be fixed....
>
> Ok, but IMO the "remove property if no value is provided for it"
> behavior needs to stay.
>
Yes, interestingly a lot of the integration tests fail otherwise -
which really surprises me. But that's a different story.
I think we should deprecate @IgnoreBlanks, properly document how blank
values are really handled, and then implement SLING-2534 which is the
opposite of @IgnoreBlanks
Carsten
--
Carsten Ziegeler
cziegeler@apache.org
Re: Empty string property values
Posted by Justin Edelson <ju...@justinedelson.com>.
On Mon, Aug 13, 2012 at 8:24 AM, Bertrand Delacretaz <bdelacretaz@apache.org
> wrote:
> On Mon, Aug 13, 2012 at 2:14 PM, Carsten Ziegeler <cz...@apache.org>
> wrote:
> > 2012/8/13 Bertrand Delacretaz <bd...@apache.org>:
>
> >> ...The first SLING-1412 commit is svn revision 916419, and the
> >> SlingDefaultValuesTest#testDefaultBehaviour test hasn't changed since
> >> rev 656302, so I think the "remove property on empty value" behavior
> >> has been there from rev 656302 or even earlier.
> >
> > Yes, I have the same feeling, but then why has SLING-1412 filed? :)
>
> IIUC the use case for that is a request with the following parameters
>
> /stringProperty@TypeHint=String[]
> ./stringProperty@IgnoreBlanks=true
> ./stringProperty=foo
> ./stringProperty=bar
> ./stringProperty=
>
> which should set only two values in the stringProperty multi-value
> field, instead of three without @IgnoreBlanks.
>
Yes - this was the intended use case.
>
> If I'm right we could probably restrict IgnoreBlanks to multi-value
> properties, if that helps clarifying things.
>
> > ...the current code only implements some strange behaviour for
> > @IgnoreBlanks which in one way or the other has to be fixed....
>
> Ok, but IMO the "remove property if no value is provided for it"
> behavior needs to stay.
>
+1
Justin
>
> -Bertrand
>
Re: Empty string property values
Posted by Bertrand Delacretaz <bd...@apache.org>.
On Mon, Aug 13, 2012 at 2:14 PM, Carsten Ziegeler <cz...@apache.org> wrote:
> 2012/8/13 Bertrand Delacretaz <bd...@apache.org>:
>> ...The first SLING-1412 commit is svn revision 916419, and the
>> SlingDefaultValuesTest#testDefaultBehaviour test hasn't changed since
>> rev 656302, so I think the "remove property on empty value" behavior
>> has been there from rev 656302 or even earlier.
>
> Yes, I have the same feeling, but then why has SLING-1412 filed? :)
IIUC the use case for that is a request with the following parameters
/stringProperty@TypeHint=String[]
./stringProperty@IgnoreBlanks=true
./stringProperty=foo
./stringProperty=bar
./stringProperty=
which should set only two values in the stringProperty multi-value
field, instead of three without @IgnoreBlanks.
If I'm right we could probably restrict IgnoreBlanks to multi-value
properties, if that helps clarifying things.
> ...the current code only implements some strange behaviour for
> @IgnoreBlanks which in one way or the other has to be fixed....
Ok, but IMO the "remove property if no value is provided for it"
behavior needs to stay.
-Bertrand
Re: Empty string property values
Posted by Carsten Ziegeler <cz...@apache.org>.
2012/8/13 Bertrand Delacretaz <bd...@apache.org>:
> On Mon, Aug 13, 2012 at 1:15 PM, Carsten Ziegeler <cz...@apache.org> wrote:
>> 2012/8/13 Bertrand Delacretaz <bd...@apache.org>:
>>> On Mon, Aug 13, 2012 at 11:47 AM, Carsten Ziegeler <cz...@apache.org> wrote:
>>>> ...after fixing these I run into failing integration tests, e.g.
>>>> SlingDefaultValuesTest#testDefaultBehaviour posts a new property with
>>>> an empty value and checks if the property does not exists afterwards....
>>>
>>> IIRC (but that was a long time ago) that's by design. Would we have
>>> another way of removing a property via a POST than setting it empty
>>> anyways?
>>>
>> See http://sling.apache.org/site/manipulating-content-the-slingpostservlet-servletspost.html
>>
>> which mentions that @IgnoreBlanks should be used instead
>
> I don't think IgnoreBlanks can be used to remove properties, IIUC it
> just causes request parameters with empty values to be ignored. So IMO
> there would be no way to remove a property if we changed the current
> behavior.
No, I think it's the other way round! At least looking at the docs and
the implementation (though buggy) I get the feeling that without
@IgnoreBlanks a blank field is not removing the property but setting
it to empty.
>
>>
>> Reading https://issues.apache.org/jira/browse/SLING-1412 where this
>> was introduced, it suggests that the current behaviour was not how
>> these things have been handled before the fix.
>
> The first SLING-1412 commit is svn revision 916419, and the
> SlingDefaultValuesTest#testDefaultBehaviour test hasn't changed since
> rev 656302, so I think the "remove property on empty value" behavior
> has been there from rev 656302 or even earlier.
Yes, I have the same feeling, but then why has SLING-1412 filed? :)
>
> IMO the current code is fine, we might just need to update the
> @IgnoreBlanks doc (or better: add integration tests for that, and yes
> I volunteer ;-)
No, the current code only implements some strange behaviour for
@IgnoreBlanks which in one way or the other has to be fixed.
Regards
Carsten
>
> -Bertrand
--
Carsten Ziegeler
cziegeler@apache.org
Re: Empty string property values
Posted by Bertrand Delacretaz <bd...@apache.org>.
On Mon, Aug 13, 2012 at 1:15 PM, Carsten Ziegeler <cz...@apache.org> wrote:
> 2012/8/13 Bertrand Delacretaz <bd...@apache.org>:
>> On Mon, Aug 13, 2012 at 11:47 AM, Carsten Ziegeler <cz...@apache.org> wrote:
>>> ...after fixing these I run into failing integration tests, e.g.
>>> SlingDefaultValuesTest#testDefaultBehaviour posts a new property with
>>> an empty value and checks if the property does not exists afterwards....
>>
>> IIRC (but that was a long time ago) that's by design. Would we have
>> another way of removing a property via a POST than setting it empty
>> anyways?
>>
> See http://sling.apache.org/site/manipulating-content-the-slingpostservlet-servletspost.html
>
> which mentions that @IgnoreBlanks should be used instead
I don't think IgnoreBlanks can be used to remove properties, IIUC it
just causes request parameters with empty values to be ignored. So IMO
there would be no way to remove a property if we changed the current
behavior.
>
> Reading https://issues.apache.org/jira/browse/SLING-1412 where this
> was introduced, it suggests that the current behaviour was not how
> these things have been handled before the fix.
The first SLING-1412 commit is svn revision 916419, and the
SlingDefaultValuesTest#testDefaultBehaviour test hasn't changed since
rev 656302, so I think the "remove property on empty value" behavior
has been there from rev 656302 or even earlier.
IMO the current code is fine, we might just need to update the
@IgnoreBlanks doc (or better: add integration tests for that, and yes
I volunteer ;-)
-Bertrand
Re: Empty string property values
Posted by Felix Meschberger <fm...@adobe.com>.
Hi,
ANd it must not be forgotten that the Sling POST Servlet was designed with first class support for HTML Forms in mind. If you consider such a form, you could pre-fill it with the current values and the user could clear the fields assuming to remove/clear the properties. So the Sling POST Servlet just removes the property.
And even though the doc may contradict the implementation, the implementation has been around for more than two years. We cannot just change the implementation without risking breaking existing applications. In this special case we should therefore fix the doc and referring to the problem.
Regards
Felix
Am 13.08.2012 um 13:15 schrieb Carsten Ziegeler:
> 2012/8/13 Bertrand Delacretaz <bd...@apache.org>:
>> On Mon, Aug 13, 2012 at 11:47 AM, Carsten Ziegeler <cz...@apache.org> wrote:
>>> ...after fixing these I run into failing integration tests, e.g.
>>> SlingDefaultValuesTest#testDefaultBehaviour posts a new property with
>>> an empty value and checks if the property does not exists afterwards....
>>
>> IIRC (but that was a long time ago) that's by design. Would we have
>> another way of removing a property via a POST than setting it empty
>> anyways?
>>
> See http://sling.apache.org/site/manipulating-content-the-slingpostservlet-servletspost.html
>
> which mentions that @IgnoreBlanks should be used instead
>
> Reading https://issues.apache.org/jira/browse/SLING-1412 where this
> was introduced, it suggests that the current behaviour was not how
> these things have been handled before the fix.
>
> Carsten
>
>
> --
> Carsten Ziegeler
> cziegeler@apache.org
Re: Empty string property values
Posted by Carsten Ziegeler <cz...@apache.org>.
2012/8/13 Bertrand Delacretaz <bd...@apache.org>:
> On Mon, Aug 13, 2012 at 11:47 AM, Carsten Ziegeler <cz...@apache.org> wrote:
>> ...after fixing these I run into failing integration tests, e.g.
>> SlingDefaultValuesTest#testDefaultBehaviour posts a new property with
>> an empty value and checks if the property does not exists afterwards....
>
> IIRC (but that was a long time ago) that's by design. Would we have
> another way of removing a property via a POST than setting it empty
> anyways?
>
See http://sling.apache.org/site/manipulating-content-the-slingpostservlet-servletspost.html
which mentions that @IgnoreBlanks should be used instead
Reading https://issues.apache.org/jira/browse/SLING-1412 where this
was introduced, it suggests that the current behaviour was not how
these things have been handled before the fix.
Carsten
--
Carsten Ziegeler
cziegeler@apache.org
Re: Empty string property values
Posted by Bertrand Delacretaz <bd...@apache.org>.
On Mon, Aug 13, 2012 at 11:47 AM, Carsten Ziegeler <cz...@apache.org> wrote:
> ...after fixing these I run into failing integration tests, e.g.
> SlingDefaultValuesTest#testDefaultBehaviour posts a new property with
> an empty value and checks if the property does not exists afterwards....
IIRC (but that was a long time ago) that's by design. Would we have
another way of removing a property via a POST than setting it empty
anyways?
-Bertrand
RE: Empty string property values
Posted by Jeff Young <je...@adobe.com>.
Hi Carsten,
While I don't have any advice on how to proceed, I can tell you that I'd be getting cold feet at about this point too. ;)
Jeff.
> -----Original Message-----
> From: Carsten Ziegeler [mailto:cziegeler@apache.org]
> Sent: 13 August 2012 10:48
> To: dev@sling.apache.org
> Subject: Re: Empty string property values
>
> While looking into SLING-2534, it seems that (as Alex noted) our docs
> say something different than the implementation.
> So I tried to fix the implementation (whic is fairly easy), but then I
> ran into failing junit tests - I think these tests have wrong
> assumptions.
> So after fixing these I run into failing integration tests, e.g.
> SlingDefaultValuesTest#testDefaultBehaviour posts a new property with
> an empty value and checks if the property does not exists afterwards.
> Now, these tests can be fixed as well, but I'm wondering if that's a
> good thing - it would be correct, but existing applications might rely
> on the wrong behaviour?
> On the other hand for rendering a missing value and an empty value
> usually doesn't make a difference...while it might make a difference
> for other apps
>
> Carsten
>
>
> 2012/7/19 Alexander Klimetschek <ak...@adobe.com>:
> > On 16.07.2012, at 17:07, Jeremy Booth wrote:
> >> For our app we do need to
> >> differentiate between provided empty and not provided.
> >
> > I can imagine that this makes sense sometimes (albeit I haven't come across
> this yet).
> >
> >> On 16 July 2012 15:42, Carsten Ziegeler <cz...@apache.org> wrote:
> >>> the handling of the empty value happens in the Sling Post servlet -
> >>> afaik this was a decision in the early days of the post servlet to
> >>> treat an empty value like no value.
> >
> > Yes, that was always in the code (I remember refactoring that once for the
> @Patch feature). It's clearly commented, albeit the initial reasoning seems
> lost:
> >
> > // if no value is present or a single empty string is given,
> > // just remove the existing property (if any)
> > removeProperty(parent, prop);
> >
> > However, the sling post servlet docs say something different:
> >
> > http://sling.apache.org/site/manipulating-content-the-slingpostservlet-
> servletspost.html#ManipulatingContent-TheSlingPostServlet%28servlets.post%29-
> %7B%7B@IgnoreBlanks%7D%7D
> >
> > The 2nd example in that section:
> >
> > <form method="POST" action="/content/page/first" enctype="multipart/form-
> data">
> > <input type="hidden" name="stringProperty@TypeHint" value="String"/>
> > <input type="text" name="stringProperty" value=""/>
> > </form>
> >
> > This will _not_ result in an empty property, but in no property at all (just
> tested it).
> >
> > Without knowing the original reasoning for "" == null, I would say the sling
> post servlet documentation is king and this is a bug :-)
> >
> > But one needs to be careful here, as there are so many options involved:
> single- vs. multi-value properties, @IgnoreBlanks, @DefaultValue = :null,
> @DefaultValue = :ignore ... (see also RequestPropertyTest).
> >
> > Cheers,
> > Alex
> >
>
>
>
> --
> Carsten Ziegeler
> cziegeler@apache.org