You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@jena.apache.org by rv...@apache.org on 2016/08/02 10:25:29 UTC
jena git commit: Avoid possible NPE in OpVisitor
Repository: jena
Updated Branches:
refs/heads/master 61368dfa0 -> c21db9408
Avoid possible NPE in OpVisitor
The default for visit(OpExt) assumed an effectiveOp() is always
available which may not be the case and can cause a NPE. Check that the
effectiveOp() is non-null and only visits in that case.
Project: http://git-wip-us.apache.org/repos/asf/jena/repo
Commit: http://git-wip-us.apache.org/repos/asf/jena/commit/c21db940
Tree: http://git-wip-us.apache.org/repos/asf/jena/tree/c21db940
Diff: http://git-wip-us.apache.org/repos/asf/jena/diff/c21db940
Branch: refs/heads/master
Commit: c21db9408a231fbb98173061d048931a8e71dac4
Parents: 61368df
Author: Rob Vesse <rv...@apache.org>
Authored: Tue Aug 2 11:24:26 2016 +0100
Committer: Rob Vesse <rv...@apache.org>
Committed: Tue Aug 2 11:24:26 2016 +0100
----------------------------------------------------------------------
.../src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/jena/blob/c21db940/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
----------------------------------------------------------------------
diff --git a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
index f947eca..ef3e952 100644
--- a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
+++ b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
@@ -57,7 +57,9 @@ public interface OpVisitor
public void visit(OpDisjunction opDisjunction) ;
public default void visit(OpExt opExt) {
- opExt.effectiveOp().visit(this);
+ Op effective = opExt.effectiveOp();
+ if (effective != null)
+ effective.visit(this);
}
// OpModifier
Re: jena git commit: Avoid possible NPE in OpVisitor
Posted by Andy Seaborne <an...@apache.org>.
On 05/08/16 09:45, Rob Vesse wrote:
> So perhaps a compromise is best. I will fix Jena so that if no
> effective operator is provided then no NPE will result. However I will
> also update the Java doc on OpExt to state that if no effective operator
> is provided implementations do so at their own risk and it may lead to
> incorrect/Incomplete query optimisation.
>
> Does that seem reasonable?
Yes - just the documentation change would be good.
Do your extensions rely on effectiveOp->null ?
I don't think there are will be any NPEs at the moment though obviously
any custom operators have to do various things to play properly.
[There seems to be 2 "OpVisitorByType", old and new (new is the one in
the walker package)]
Andy
>
> Rob
>
> On 04/08/2016 14:33, "Andy Seaborne" <an...@apache.org> wrote:
>
> On 02/08/16 17:01, Rob Vesse wrote:
> > Potentially yes
> >
> > However this was the only source of errors when I upgraded our code
> > to the latest builds. I will try and concoct some additional test
> > cases against our code to see if I can hit any of those other places.
> > We don\u2019t use join classification in our optimiser so we are unlikely
> > to ever hit that case. We do actually use the variable finder and
> > once this case was fixed that did not elicit any further issues.
> >
> > There is a general question here about the use of OpExt, if the
> > assumption is that it simply abstracts some specialism of an algebra
> > then we will fail that assumption. We use it to add additional
> > features to the query language Beyond what vanilla ARQ it Is able to
> > support and so there is no effective operator.
>
> Returning (table unit) would be possible.
>
> The main thing is as a way to declare the variables and how they are
> used by the OpExt. The effective Op is not used for execution at all.
>
> Andy
>
> >
> > Rob
> >
> > On 02/08/2016 16:19, "Andy Seaborne" <an...@apache.org> wrote:
> >
> > Rob,
> >
> > Several places in the code access effectiveOp during optimization depend
> > and on there being an equivalent non-null op (e.g. variable finding and
> > classification).
> >
> > Aren't they vulnerable?
> >
> > Andy
> >
> > On 02/08/16 11:25, rvesse@apache.org wrote:
> > > Repository: jena
> > > Updated Branches:
> > > refs/heads/master 61368dfa0 -> c21db9408
> > >
> > >
> > > Avoid possible NPE in OpVisitor
> > >
> > > The default for visit(OpExt) assumed an effectiveOp() is always
> > > available which may not be the case and can cause a NPE. Check that the
> > > effectiveOp() is non-null and only visits in that case.
> > >
> > >
> > > Project: http://git-wip-us.apache.org/repos/asf/jena/repo
> > > Commit: http://git-wip-us.apache.org/repos/asf/jena/commit/c21db940
> > > Tree: http://git-wip-us.apache.org/repos/asf/jena/tree/c21db940
> > > Diff: http://git-wip-us.apache.org/repos/asf/jena/diff/c21db940
> > >
> > > Branch: refs/heads/master
> > > Commit: c21db9408a231fbb98173061d048931a8e71dac4
> > > Parents: 61368df
> > > Author: Rob Vesse <rv...@apache.org>
> > > Authored: Tue Aug 2 11:24:26 2016 +0100
> > > Committer: Rob Vesse <rv...@apache.org>
> > > Committed: Tue Aug 2 11:24:26 2016 +0100
> > >
> > > ----------------------------------------------------------------------
> > > .../src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > ----------------------------------------------------------------------
> > >
> > >
> > > http://git-wip-us.apache.org/repos/asf/jena/blob/c21db940/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> > > ----------------------------------------------------------------------
> > > diff --git a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> > > index f947eca..ef3e952 100644
> > > --- a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> > > +++ b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> > > @@ -57,7 +57,9 @@ public interface OpVisitor
> > > public void visit(OpDisjunction opDisjunction) ;
> > >
> > > public default void visit(OpExt opExt) {
> > > - opExt.effectiveOp().visit(this);
> > > + Op effective = opExt.effectiveOp();
> > > + if (effective != null)
> > > + effective.visit(this);
> > > }
> > >
> > > // OpModifier
> > >
> >
> >
> >
> >
> >
> >
>
>
>
>
>
>
Re: jena git commit: Avoid possible NPE in OpVisitor
Posted by Rob Vesse <rv...@dotnetrdf.org>.
So perhaps a compromise is best. I will fix Jena so that if no effective operator is provided then no NPE will result. However I will also update the Java doc on OpExt to state that if no effective operator is provided implementations do so at their own risk and it may lead to incorrect/Incomplete query optimisation.
Does that seem reasonable?
Rob
On 04/08/2016 14:33, "Andy Seaborne" <an...@apache.org> wrote:
On 02/08/16 17:01, Rob Vesse wrote:
> Potentially yes
>
> However this was the only source of errors when I upgraded our code
> to the latest builds. I will try and concoct some additional test
> cases against our code to see if I can hit any of those other places.
> We don’t use join classification in our optimiser so we are unlikely
> to ever hit that case. We do actually use the variable finder and
> once this case was fixed that did not elicit any further issues.
>
> There is a general question here about the use of OpExt, if the
> assumption is that it simply abstracts some specialism of an algebra
> then we will fail that assumption. We use it to add additional
> features to the query language Beyond what vanilla ARQ it Is able to
> support and so there is no effective operator.
Returning (table unit) would be possible.
The main thing is as a way to declare the variables and how they are
used by the OpExt. The effective Op is not used for execution at all.
Andy
>
> Rob
>
> On 02/08/2016 16:19, "Andy Seaborne" <an...@apache.org> wrote:
>
> Rob,
>
> Several places in the code access effectiveOp during optimization depend
> and on there being an equivalent non-null op (e.g. variable finding and
> classification).
>
> Aren't they vulnerable?
>
> Andy
>
> On 02/08/16 11:25, rvesse@apache.org wrote:
> > Repository: jena
> > Updated Branches:
> > refs/heads/master 61368dfa0 -> c21db9408
> >
> >
> > Avoid possible NPE in OpVisitor
> >
> > The default for visit(OpExt) assumed an effectiveOp() is always
> > available which may not be the case and can cause a NPE. Check that the
> > effectiveOp() is non-null and only visits in that case.
> >
> >
> > Project: http://git-wip-us.apache.org/repos/asf/jena/repo
> > Commit: http://git-wip-us.apache.org/repos/asf/jena/commit/c21db940
> > Tree: http://git-wip-us.apache.org/repos/asf/jena/tree/c21db940
> > Diff: http://git-wip-us.apache.org/repos/asf/jena/diff/c21db940
> >
> > Branch: refs/heads/master
> > Commit: c21db9408a231fbb98173061d048931a8e71dac4
> > Parents: 61368df
> > Author: Rob Vesse <rv...@apache.org>
> > Authored: Tue Aug 2 11:24:26 2016 +0100
> > Committer: Rob Vesse <rv...@apache.org>
> > Committed: Tue Aug 2 11:24:26 2016 +0100
> >
> > ----------------------------------------------------------------------
> > .../src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> > ----------------------------------------------------------------------
> >
> >
> > http://git-wip-us.apache.org/repos/asf/jena/blob/c21db940/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> > ----------------------------------------------------------------------
> > diff --git a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> > index f947eca..ef3e952 100644
> > --- a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> > +++ b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> > @@ -57,7 +57,9 @@ public interface OpVisitor
> > public void visit(OpDisjunction opDisjunction) ;
> >
> > public default void visit(OpExt opExt) {
> > - opExt.effectiveOp().visit(this);
> > + Op effective = opExt.effectiveOp();
> > + if (effective != null)
> > + effective.visit(this);
> > }
> >
> > // OpModifier
> >
>
>
>
>
>
>
Re: jena git commit: Avoid possible NPE in OpVisitor
Posted by Andy Seaborne <an...@apache.org>.
On 02/08/16 17:01, Rob Vesse wrote:
> Potentially yes
>
> However this was the only source of errors when I upgraded our code
> to the latest builds. I will try and concoct some additional test
> cases against our code to see if I can hit any of those other places.
> We don\u2019t use join classification in our optimiser so we are unlikely
> to ever hit that case. We do actually use the variable finder and
> once this case was fixed that did not elicit any further issues.
>
> There is a general question here about the use of OpExt, if the
> assumption is that it simply abstracts some specialism of an algebra
> then we will fail that assumption. We use it to add additional
> features to the query language Beyond what vanilla ARQ it Is able to
> support and so there is no effective operator.
Returning (table unit) would be possible.
The main thing is as a way to declare the variables and how they are
used by the OpExt. The effective Op is not used for execution at all.
Andy
>
> Rob
>
> On 02/08/2016 16:19, "Andy Seaborne" <an...@apache.org> wrote:
>
> Rob,
>
> Several places in the code access effectiveOp during optimization depend
> and on there being an equivalent non-null op (e.g. variable finding and
> classification).
>
> Aren't they vulnerable?
>
> Andy
>
> On 02/08/16 11:25, rvesse@apache.org wrote:
> > Repository: jena
> > Updated Branches:
> > refs/heads/master 61368dfa0 -> c21db9408
> >
> >
> > Avoid possible NPE in OpVisitor
> >
> > The default for visit(OpExt) assumed an effectiveOp() is always
> > available which may not be the case and can cause a NPE. Check that the
> > effectiveOp() is non-null and only visits in that case.
> >
> >
> > Project: http://git-wip-us.apache.org/repos/asf/jena/repo
> > Commit: http://git-wip-us.apache.org/repos/asf/jena/commit/c21db940
> > Tree: http://git-wip-us.apache.org/repos/asf/jena/tree/c21db940
> > Diff: http://git-wip-us.apache.org/repos/asf/jena/diff/c21db940
> >
> > Branch: refs/heads/master
> > Commit: c21db9408a231fbb98173061d048931a8e71dac4
> > Parents: 61368df
> > Author: Rob Vesse <rv...@apache.org>
> > Authored: Tue Aug 2 11:24:26 2016 +0100
> > Committer: Rob Vesse <rv...@apache.org>
> > Committed: Tue Aug 2 11:24:26 2016 +0100
> >
> > ----------------------------------------------------------------------
> > .../src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> > ----------------------------------------------------------------------
> >
> >
> > http://git-wip-us.apache.org/repos/asf/jena/blob/c21db940/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> > ----------------------------------------------------------------------
> > diff --git a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> > index f947eca..ef3e952 100644
> > --- a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> > +++ b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> > @@ -57,7 +57,9 @@ public interface OpVisitor
> > public void visit(OpDisjunction opDisjunction) ;
> >
> > public default void visit(OpExt opExt) {
> > - opExt.effectiveOp().visit(this);
> > + Op effective = opExt.effectiveOp();
> > + if (effective != null)
> > + effective.visit(this);
> > }
> >
> > // OpModifier
> >
>
>
>
>
>
>
Re: jena git commit: Avoid possible NPE in OpVisitor
Posted by Rob Vesse <rv...@dotnetrdf.org>.
Potentially yes
However this was the only source of errors when I upgraded our code to the latest builds. I will try and concoct some additional test cases against our code to see if I can hit any of those other places. We don’t use join classification in our optimiser so we are unlikely to ever hit that case. We do actually use the variable finder and once this case was fixed that did not elicit any further issues.
There is a general question here about the use of OpExt, if the assumption is that it simply abstracts some specialism of an algebra then we will fail that assumption. We use it to add additional features to the query language Beyond what vanilla ARQ it Is able to support and so there is no effective operator.
Rob
On 02/08/2016 16:19, "Andy Seaborne" <an...@apache.org> wrote:
Rob,
Several places in the code access effectiveOp during optimization depend
and on there being an equivalent non-null op (e.g. variable finding and
classification).
Aren't they vulnerable?
Andy
On 02/08/16 11:25, rvesse@apache.org wrote:
> Repository: jena
> Updated Branches:
> refs/heads/master 61368dfa0 -> c21db9408
>
>
> Avoid possible NPE in OpVisitor
>
> The default for visit(OpExt) assumed an effectiveOp() is always
> available which may not be the case and can cause a NPE. Check that the
> effectiveOp() is non-null and only visits in that case.
>
>
> Project: http://git-wip-us.apache.org/repos/asf/jena/repo
> Commit: http://git-wip-us.apache.org/repos/asf/jena/commit/c21db940
> Tree: http://git-wip-us.apache.org/repos/asf/jena/tree/c21db940
> Diff: http://git-wip-us.apache.org/repos/asf/jena/diff/c21db940
>
> Branch: refs/heads/master
> Commit: c21db9408a231fbb98173061d048931a8e71dac4
> Parents: 61368df
> Author: Rob Vesse <rv...@apache.org>
> Authored: Tue Aug 2 11:24:26 2016 +0100
> Committer: Rob Vesse <rv...@apache.org>
> Committed: Tue Aug 2 11:24:26 2016 +0100
>
> ----------------------------------------------------------------------
> .../src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/jena/blob/c21db940/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> ----------------------------------------------------------------------
> diff --git a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> index f947eca..ef3e952 100644
> --- a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> +++ b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> @@ -57,7 +57,9 @@ public interface OpVisitor
> public void visit(OpDisjunction opDisjunction) ;
>
> public default void visit(OpExt opExt) {
> - opExt.effectiveOp().visit(this);
> + Op effective = opExt.effectiveOp();
> + if (effective != null)
> + effective.visit(this);
> }
>
> // OpModifier
>
Re: jena git commit: Avoid possible NPE in OpVisitor
Posted by Andy Seaborne <an...@apache.org>.
Rob,
Several places in the code access effectiveOp during optimization depend
and on there being an equivalent non-null op (e.g. variable finding and
classification).
Aren't they vulnerable?
Andy
On 02/08/16 11:25, rvesse@apache.org wrote:
> Repository: jena
> Updated Branches:
> refs/heads/master 61368dfa0 -> c21db9408
>
>
> Avoid possible NPE in OpVisitor
>
> The default for visit(OpExt) assumed an effectiveOp() is always
> available which may not be the case and can cause a NPE. Check that the
> effectiveOp() is non-null and only visits in that case.
>
>
> Project: http://git-wip-us.apache.org/repos/asf/jena/repo
> Commit: http://git-wip-us.apache.org/repos/asf/jena/commit/c21db940
> Tree: http://git-wip-us.apache.org/repos/asf/jena/tree/c21db940
> Diff: http://git-wip-us.apache.org/repos/asf/jena/diff/c21db940
>
> Branch: refs/heads/master
> Commit: c21db9408a231fbb98173061d048931a8e71dac4
> Parents: 61368df
> Author: Rob Vesse <rv...@apache.org>
> Authored: Tue Aug 2 11:24:26 2016 +0100
> Committer: Rob Vesse <rv...@apache.org>
> Committed: Tue Aug 2 11:24:26 2016 +0100
>
> ----------------------------------------------------------------------
> .../src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/jena/blob/c21db940/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> ----------------------------------------------------------------------
> diff --git a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> index f947eca..ef3e952 100644
> --- a/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> +++ b/jena-arq/src/main/java/org/apache/jena/sparql/algebra/OpVisitor.java
> @@ -57,7 +57,9 @@ public interface OpVisitor
> public void visit(OpDisjunction opDisjunction) ;
>
> public default void visit(OpExt opExt) {
> - opExt.effectiveOp().visit(this);
> + Op effective = opExt.effectiveOp();
> + if (effective != null)
> + effective.visit(this);
> }
>
> // OpModifier
>