Uploaded image for project: 'TinkerPop'
  1. TinkerPop
  2. TINKERPOP-1356

Several issues in HasContainer

    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 3.2.0-incubating, 3.1.2-incubating
    • Fix Version/s: 3.2.3
    • Component/s: process
    • Labels:
      None

      Description

      HasContainer has some issues that are not covered by test cases, but IMO likely to happen in real world applications.

      Empty collections lead to uncaught exceptions with a meaningless error message:

      gremlin> g = TinkerFactory.createModern().traversal()
      ==>graphtraversalsource[tinkergraph[vertices:6 edges:6], standard]
      gremlin> g.V().hasId(within(new ArrayList()))
      0
      Display stack trace? [yN] N
      gremlin> g.V().hasId(without(new ArrayList()))
      0
      Display stack trace? [yN]
      

      In the constructor of HasContainer we should use:

      ((Collection) this.predicate.getValue()).stream().findFirst().orElseGet(Object::new)
      

      ...instead of:

      ((Collection) this.predicate.getValue()).toArray()[0]
      

      ..or anything else that doesn't assume that we will always have a first element.

      Furthermore enforceHomogenousCollectionIfPresent should check for empty collections:

      ...
      final Collection collection = (Collection) predicateValue;
      if (!collection.isEmpty()) { // <--
          final Class<?> first = collection.toArray()[0].getClass();
          if (!((Collection) predicateValue).stream().map(Object::getClass).allMatch(c -> first.equals(c)))
              throw new IllegalArgumentException("Has comparisons on a collection of ids require ids to all be of the same type");
      }
      ...
      

      And the last issue is this one (from the code snippet above): collection.toArray()[0].getClass(). What if the first (or any other) element is actually null? The check should be more like:

      final Object obj = new Object();
      if (((Collection) predicateValue).stream().map(o -> Optional.ofNullable(o).orElse(obj).getClass()).distinct().count() != 1)
          throw ...
      

      Or something smarter like that, as long as it has a null-check.

      The proposed code changes seem to work fine, except for hasId(within(<empty-collection>)), which emits all vertices instead of none.

        Attachments

          Activity

            People

            • Assignee:
              dkuppitz Daniel Kuppitz
              Reporter:
              dkuppitz Daniel Kuppitz
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: