Fix JexlMap and GenotypeJEXLContext NullPtrException #668

SHuang-Broad · Jul 24, 2016

Description

GenotypeJEXLContext.get() currently potentially throws NullPointerException, triggered by a design choice in Genotype.getFilters(), which explicitly returns null (genotype considered to be filter passing in such case according to documentation). This fix temporarily fixes that exception (1st commit, by Louis).
Class CommonInfo, which holds "Common utility routines for VariantContext and Genotype", contains such a constant
```
private static Set<String> NO_FILTERS = Collections.emptySet();
```
VariantContext uses this constant in its own getFilters() (it has a separate method getFiltersMaybeNull()), but Genotype doesn't use such constant. A choice has to be made to whether make Genotype resemble VariantContext's behavior (a backward non-compatible change) or keep things as they are.
Some code re-organizing and whole bunch of todo's in JEXLMap, which must be resolved in the review rounds.

Checklist

Code compiles correctly
New tests covering changes and new functionality
All tests passing
[PartiallyDone] Extended the README / documentation, if necessary
[Possibly] Is not backward compatible (breaks binary or source compatibility)

SHuang-Broad · Jul 24, 2016

@lbergelson , would you mind open discussions on all these?
I don't think this is something that I should decide on my own.

EDIT:
I mean provide your insight, not "open discussions" as tickets...

coveralls · Jul 24, 2016

Coverage decreased (-0.0002%) to 68.42% when pulling aa23d6f on SHuang-Broad:sh_fix_GenotypeJEXLContext_nullptr into fcfad25 on samtools:master.

coveralls · Jul 26, 2016

Coverage decreased (-0.01%) to 68.447% when pulling acd4f23 on SHuang-Broad:sh_fix_GenotypeJEXLContext_nullptr into 87b1e87 on samtools:master.

coveralls · Aug 10, 2016

Coverage decreased (-0.007%) to 68.8% when pulling 7ac9a57 on SHuang-Broad:sh_fix_GenotypeJEXLContext_nullptr into c8202f2 on samtools:master.

SHuang-Broad · Aug 13, 2016

Sorry @lbergelson , I accidentally pushed another improvements that I'm working on to this branch. Please ignore the third commit first.
Sorry.

@SHuang-Broad I think it would be better to open an issue with this comment instead of putting it into the code.

Yes, I just wasn't sure if people would care.

Done, see #686.

Why move these down like this? Not a big deal either way but I don't see the reason to change them.

Will change back, thanks!

We don't typically have these sorts of big signpost comments. It seems useful for dividing the unsupported operations from the rest, but I'm not sure you need tthem for the rest of the class.

For me personally, it serves the purpose of grouping methods providing similar functionalities in proximity, so that I would jump around to see how one function works. The second reason that I put in these "block segregators" is such that the interfaces exposed and the implementations can be better separated with a quick glance.

If you think this clusters the class instead of helping, I'll remove them.

Removed, but kept the structure of "public first, private later".

Does this also throw IllegalArgumentException in other cases like you mentioned in the todo? Can you describe those cases?

Done.

Can you describe when it throws IllegalArgumentException, which is strange since it takes no arguments.

Done.

hold -> held

Done

How can you know if they're invalid? By evaluating them and seeing if they throw?

This echoes the "todo" comment at the beginning of this class. There might be no way of knowing if they invalid unless they are evaluated.

I'm not sure this is any clearer than just initialize. Comment might read better as
"Initializes all keys with null values indicating that they have not yet been evaluated. The actual value will be computed only when the key is requested"

Done.

I think this was clearer with three distinct cases instead of collapsing one of them.

Done.

delete this one if it's unused, it's private so nothing else could be using it.

Done.

These are fine I think. There isn't a good way to convert these to a compile errors since java doesn't have separate interfaces for immutable collections. It's pretty standard to implement some collection functions to just throw UnsupportedOperationException

Got it. Thanks!

In the previous version this handled exceptions that had "undefined variable" differently than other exceptions. I think you should have a separate catch block for JexlException.Variable that does jexl.put(exp,false) and then the catch for generic JexlException that get translated to IllegalArgumentException

Good point! Done.

coveralls · Aug 18, 2016

Coverage decreased (-0.0002%) to 68.807% when pulling 1ddd123 on SHuang-Broad:sh_fix_GenotypeJEXLContext_nullptr into c8202f2 on samtools:master.

could you replace this throw with

throw new IllegalArgumentexception(String.format("Invalid JEXL expression detected for %s", exp.name), e)

so that the underlying cause is propagated more cleanly?

done

we like to use

} else if {

(here and elsewhere)

and

} else {

done

spaces:

if␣(name␣==␣null)␣{....

done

spaces here too

done

lbergelson · Aug 23, 2016

@SHuang-Broad Can you add a unit test that used to fail with nullptr?

space before brace

done

space

done

SHuang-Broad · Aug 23, 2016

@lbergelson , yes, will add a test.

coveralls · Aug 23, 2016

Coverage increased (+0.006%) to 68.887% when pulling 2cc2b6c on SHuang-Broad:sh_fix_GenotypeJEXLContext_nullptr into a31f6c8 on samtools:master.

SHuang-Broad · Aug 27, 2016

There you go, @lbergelson , the unit test were cleaned up a little and more tests are added.
Thanks!

coveralls · Aug 27, 2016

Coverage increased (+0.09%) to 68.974% when pulling dd4fc60 on SHuang-Broad:sh_fix_GenotypeJEXLContext_nullptr into a31f6c8 on samtools:master.

SHuang-Broad · Sep 7, 2016

Friendly poke on @lbergelson, is the unit test covering what's needed?

coveralls · Sep 8, 2016

Coverage increased (+0.1%) to 68.996% when pulling b67aea1 on SHuang-Broad:sh_fix_GenotypeJEXLContext_nullptr into a31f6c8 on samtools:master.

lbergelson · Sep 13, 2016

@SHuang-Broad This looks good. Sorry for slowness. 👍

SHuang-Broad · Sep 13, 2016

Thank you very much! @lbergelson

lbergelson was assigned by droazen Aug 9, 2016

droazen referenced this pull request Aug 9, 2016
Open
Review "Party" #548

yfarjoun added the Review-party candidate label Aug 9, 2016

SHuang-Broad added some commits Jul 15, 2016

SHuang-Broad initial fix from Louis
cleaned up
708a6e7

SHuang-Broad cleaned up db38c95

SHuang-Broad Review comments addressed; many to do removed; evaluateExpression() i…
…mproved
0e7a48e

SHuang-Broad changed the title from NO merge yet: Fix JexlMap and GenotypeJEXLContext NullPtrException to Fix JexlMap and GenotypeJEXLContext NullPtrException Aug 22, 2016

SHuang-Broad coding style (spaces) change

All checks have passed
2 successful checks

continuous-integration/travis-ci/pr — The Travis CI build passed

Details

coverage/coveralls — Coverage increased (+0.006%) to 68.887%

Details

2cc2b6c

SHuang-Broad unit test cleaned up and added more

All checks have passed
2 successful checks

continuous-integration/travis-ci/pr — The Travis CI build passed

Details

coverage/coveralls — Coverage increased (+0.09%) to 68.974%

Details

dd4fc60

SHuang-Broad found how to test Genotype equality and added test case

All checks have passed
2 successful checks

continuous-integration/travis-ci/pr — The Travis CI build passed

Details

coverage/coveralls — Coverage increased (+0.1%) to 68.996%

Details

b67aea1

lbergelson merged commit 21d8865 into samtools:master Sep 13, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

coverage/coveralls Coverage increased (+0.1%) to 68.996%
Details

SHuang-Broad deleted the SHuang-Broad:sh_fix_GenotypeJEXLContext_nullptr branch Sep 13, 2016

          /**
-    -     * create the internal JexlContext, only when required.  This code is where new JEXL context variables
-    -     * should get added.
-    -     *
+    +     * Note: due to laziness, this accessor actually modifies the instance by possibly forcing evaluation of an Jexl expression.
+    +     * @throws IllegalArgumentException when {@code o} is {@code null}

              return jexl.values();
          }
          /**
-    -     * evaulate a JexlVCMatchExp's expression, given the current context (and setup the context if it's null)
-    -     * @param exp the JexlVCMatchExp to evaluate
+    +     * TODO: the number may count invalid entries, is this good?
+    +     * @return the number of keys, i.e. {@link JexlVCMatchExp}'s hold by this mapping.

              return jexl.values();
          }
          /**
-    -     * evaulate a JexlVCMatchExp's expression, given the current context (and setup the context if it's null)
-    -     * @param exp the JexlVCMatchExp to evaluate
+    +     * TODO: the number may count invalid entries, is this good?

+    +        return jexl.put(jexlVCMatchExp, aBoolean);
+    +    }
+    +
+    +    public void putAll(Map<? extends JexlVCMatchExp, ? extends Boolean> map) {
+    +        jexl.putAll(map);
+    +    }
+    +
+    +    // -----------------------------------------------------------------------------------------------
+    +    // Utilities
+    +    // -----------------------------------------------------------------------------------------------
+    +
+    +    /**
+    +     * Lazily initializes {@link #jexl}, in the sense that all keys in {@code jexlCollection} are associated with {@code null}.
+    +     * @param jexlCollection
+    +     */
+    +    private void lazyInitialize(Collection<JexlVCMatchExp> jexlCollection) {

-    -            if (e.getMessage() != null && e.getMessage().contains("undefined variable"))
-    -                jexl.put(exp,false);
-    -            else
-    -                throw new IllegalArgumentException(String.format("Invalid JEXL expression detected for %s with message %s", exp.name, (e.getMessage() == null ? "no message" : e.getMessage())));
+    +            // if exception happens because variable is undefined (i.e. field in expression is not present), evaluate to FALSE
+    +            throw new IllegalArgumentException(String.format("Invalid JEXL expression detected for %s with message %s",
+    +                    exp.name,
+    +                        /*(e.getMessage() == null ? */"no message" /*: e.getMessage())*/));
+    +        }
+    +    }
+    +
+    +    /**
+    +     * Create the internal JexlContext, only when required.
+    +     * This code is where new JEXL context variables should get added.
+    +     */
+    +    private void createContext() {

samtools/htsjdk

Join GitHub today

Fix JexlMap and GenotypeJEXLContext NullPtrException #668

Conversation

SHuang-Broad commented Jul 24, 2016

Description

Checklist

SHuang-Broad commented Jul 24, 2016 • edited

coveralls commented Jul 24, 2016

coveralls commented Jul 26, 2016

lbergelson was assigned by droazen Aug 9, 2016

droazen referenced this pull request Aug 9, 2016

yfarjoun added the Review-party candidate label Aug 9, 2016

coveralls commented Aug 10, 2016

SHuang-Broad commented Aug 13, 2016

Show 3 comments Hide comments lbergelson and 1 other commented on an outdated diff Aug 16, 2016

Show 2 comments Hide comments lbergelson and 1 other commented on an outdated diff Aug 16, 2016

Show 3 comments Hide comments lbergelson and 1 other commented on an outdated diff Aug 16, 2016

Show 2 comments Hide comments lbergelson and 1 other commented on an outdated diff Aug 16, 2016

lbergelson Aug 16, 2016 • edited

Show 2 comments Hide comments lbergelson and 1 other commented on an outdated diff Aug 16, 2016

Show 2 comments Hide comments lbergelson and 1 other commented on an outdated diff Aug 16, 2016

Show 2 comments Hide comments lbergelson and 1 other commented on an outdated diff Aug 16, 2016

Show 2 comments Hide comments lbergelson and 1 other commented on an outdated diff Aug 16, 2016

Show 2 comments Hide comments lbergelson and 1 other commented on an outdated diff Aug 16, 2016

Show 2 comments Hide comments lbergelson and 1 other commented on an outdated diff Aug 16, 2016

Show 2 comments Hide comments lbergelson and 1 other commented on an outdated diff Aug 16, 2016

Show 2 comments Hide comments lbergelson and 1 other commented on an outdated diff Aug 16, 2016

coveralls commented Aug 18, 2016

Show 2 comments Hide comments lbergelson and 1 other commented on an outdated diff Aug 19, 2016

SHuang-Broad added some commits Jul 15, 2016

SHuang-Broad changed the title from NO merge yet: Fix JexlMap and GenotypeJEXLContext NullPtrException to Fix JexlMap and GenotypeJEXLContext NullPtrException Aug 22, 2016

Show 3 comments Hide comments yfarjoun and 1 other commented on an outdated diff Aug 23, 2016

Show 2 comments Hide comments yfarjoun and 1 other commented on an outdated diff Aug 23, 2016

Show 2 comments Hide comments yfarjoun and 1 other commented on an outdated diff Aug 23, 2016

lbergelson commented Aug 23, 2016

Show 2 comments Hide comments yfarjoun and 1 other commented on an outdated diff Aug 23, 2016

Show 2 comments Hide comments yfarjoun and 1 other commented on an outdated diff Aug 23, 2016

SHuang-Broad commented Aug 23, 2016

All checks have passed

coveralls commented Aug 23, 2016

All checks have passed

SHuang-Broad commented Aug 27, 2016

coveralls commented Aug 27, 2016

All checks have passed

SHuang-Broad commented Sep 7, 2016

coveralls commented Sep 8, 2016

lbergelson commented Sep 13, 2016

Hide details View details lbergelson merged commit 21d8865 into samtools:master Sep 13, 2016 2 checks passed

2 checks passed

SHuang-Broad deleted the SHuang-Broad:sh_fix_GenotypeJEXLContext_nullptr branch Sep 13, 2016

SHuang-Broad commented Sep 13, 2016

lbergelson and 1 other commented on an outdated diff Aug 16, 2016

lbergelson and 1 other commented on an outdated diff Aug 16, 2016

lbergelson and 1 other commented on an outdated diff Aug 16, 2016

lbergelson and 1 other commented on an outdated diff Aug 16, 2016

lbergelson and 1 other commented on an outdated diff Aug 16, 2016

lbergelson and 1 other commented on an outdated diff Aug 16, 2016

lbergelson and 1 other commented on an outdated diff Aug 16, 2016

lbergelson and 1 other commented on an outdated diff Aug 16, 2016

lbergelson and 1 other commented on an outdated diff Aug 16, 2016

lbergelson and 1 other commented on an outdated diff Aug 16, 2016

lbergelson and 1 other commented on an outdated diff Aug 16, 2016

lbergelson and 1 other commented on an outdated diff Aug 16, 2016

lbergelson and 1 other commented on an outdated diff Aug 19, 2016

yfarjoun and 1 other commented on an outdated diff Aug 23, 2016

yfarjoun and 1 other commented on an outdated diff Aug 23, 2016

yfarjoun and 1 other commented on an outdated diff Aug 23, 2016

yfarjoun and 1 other commented on an outdated diff Aug 23, 2016

yfarjoun and 1 other commented on an outdated diff Aug 23, 2016

lbergelson merged commit `21d8865` into samtools:master Sep 13, 2016
2 checks passed