assert

[Marcel Reutegger <mr...@adobe.com>]

hi,

I noticed a number of asserts in the code that tries to enforce (?)
constraints in the method contract. IMO this is problematic. assert
statements are only evaluated when enabled explicitly. usually
this only happens when tests run and a production system will
not run with asserts. this means we shouldn't use asserts to
enforce the method contract of a public method. one example
is RootImpl.getLocation(String):

    public TreeLocation getLocation(String path) {
        // TODO: must not use assertion to check contract! asserts must be enabled explicitly
        assert path.startsWith("/");
        return rootTree.getLocation().getChild(path.substring(1));
    }

the method will work correctly with assertions enabled, but fail
when assertions are disabled and the passed path is relative. you
wouldn't even notice it right away, because the method will just
cut the first character and move on.

I think we need to throw an exception in this case. IllegalArgumentException?

regards
 marcel

RE: assert

[Marcel Reutegger <mr...@adobe.com>]

OK, thanks for your feedback. I'll have a look and create
a JIRA issue.

regards
 marcel

> -----Original Message-----
> From: Michael Dürig [mailto:michid@gmail.com]
> Sent: Donnerstag, 6. September 2012 14:59
> To: oak-dev@jackrabbit.apache.org
> Subject: Re: assert
> 
> 
> Hi,
> 
> On 6.9.12 13:51, Jukka Zitting wrote:
> > Hi,
> >
> > On Thu, Sep 6, 2012 at 2:36 PM, Marcel Reutegger
> <mr...@adobe.com> wrote:
> >> I think we need to throw an exception in this case.
> IllegalArgumentException?
> 
> This shouldn't be an assertion agreed.
> 
> >
> > Agreed in general. I've already encountered a few cases where a test
> > case runs just fine in Eclipse (without assertions enabled) but then
> > fails during the Maven build (which enables assertions). It would be
> > better if such failsafes were executed always unless they're too
> > expensive to compute in normal operation.
> >
> > The precondition feature [1] in Guava is a pretty nice way to
> > implement such checks.
> 
> +1 for using the precondition feature.
> 
> One notable exception might be PathUtils where the precondition check
> might turn out to be quite expensive. So I think we should keep the
> assertions there.
> 
> Michael
> 
> 
> >
> > [1] http://code.google.com/p/guava-libraries/wiki/PreconditionsExplained
> >
> > BR,
> >
> > Jukka Zitting
> >

Re: assert

[Michael Dürig <mi...@gmail.com>]

Hi,

On 6.9.12 13:51, Jukka Zitting wrote:
> Hi,
>
> On Thu, Sep 6, 2012 at 2:36 PM, Marcel Reutegger <mr...@adobe.com> wrote:
>> I think we need to throw an exception in this case. IllegalArgumentException?

This shouldn't be an assertion agreed.

>
> Agreed in general. I've already encountered a few cases where a test
> case runs just fine in Eclipse (without assertions enabled) but then
> fails during the Maven build (which enables assertions). It would be
> better if such failsafes were executed always unless they're too
> expensive to compute in normal operation.
>
> The precondition feature [1] in Guava is a pretty nice way to
> implement such checks.

+1 for using the precondition feature.

One notable exception might be PathUtils where the precondition check 
might turn out to be quite expensive. So I think we should keep the 
assertions there.

Michael


>
> [1] http://code.google.com/p/guava-libraries/wiki/PreconditionsExplained
>
> BR,
>
> Jukka Zitting
>

Re: assert

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Thu, Sep 6, 2012 at 2:36 PM, Marcel Reutegger <mr...@adobe.com> wrote:
> I think we need to throw an exception in this case. IllegalArgumentException?

Agreed in general. I've already encountered a few cases where a test
case runs just fine in Eclipse (without assertions enabled) but then
fails during the Maven build (which enables assertions). It would be
better if such failsafes were executed always unless they're too
expensive to compute in normal operation.

The precondition feature [1] in Guava is a pretty nice way to
implement such checks.

[1] http://code.google.com/p/guava-libraries/wiki/PreconditionsExplained

BR,

Jukka Zitting