Fix JavaTranslator's has handling for null arguments

[andrii0lomakin]

• Updated has() special case logic to address potential issues when first or third arguments are null. Example : g.E().has("knows","weight",null) this query can lead to NPE on server execution as JavaTranslator can choose GraphTravers#has(String, Sring, P) method instead of GraphTravers#has(String, Sring, Object)

[codecov-commenter]

Codecov Report

❌ Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (3.8-dev@3cfb322). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...pache/tinkerpop/gremlin/jsr223/JavaTranslator.java	71.42%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             3.8-dev    #3274   +/-   ##
==========================================
  Coverage           ?   77.29%           
  Complexity         ?    15015           
==========================================
  Files              ?     1159           
  Lines              ?    71926           
  Branches           ?     8025           
==========================================
  Hits               ?    55596           
  Misses             ?    13277           
  Partials           ?     3053           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[andreachild]

It would be good to add a unit test for this. Here is a suggested one since there are no existing (direct) unit tests for the JavaTranslator. Note that if you execute the test below without the code fix it will randomly fail/pass depending on the order of methods loaded into the JavaTranslator.GLOBAL_METHOD_CACHE.

package org.apache.tinkerpop.gremlin.jsr223;

import org.apache.tinkerpop.gremlin.process.traversal.Bytecode;
import org.apache.tinkerpop.gremlin.process.traversal.P;
import org.apache.tinkerpop.gremlin.process.traversal.Traversal;
import org.apache.tinkerpop.gremlin.process.traversal.dsl.graph.GraphTraversalSource;
import org.apache.tinkerpop.gremlin.structure.util.empty.EmptyGraph;
import org.junit.Test;

import static org.junit.Assert.assertEquals;

public class JavaTranslatorTest {
    private GraphTraversalSource g = EmptyGraph.instance().traversal();
    private JavaTranslator<GraphTraversalSource, Traversal.Admin<?, ?>> translator = JavaTranslator.of(EmptyGraph.instance().traversal());

    @Test
    public void shouldTranslateHasWithObjectArgValue() {
        final Bytecode bytecode = new Bytecode();
        bytecode.addStep("E");
        bytecode.addStep("has", "knows", "weight", 1.0);
        final Traversal.Admin<?, ?> translation = translator.translate(bytecode);
        assertEquals(g.E().has("knows", "weight", 1.0).asAdmin(), translation);
    }

    @Test
    public void shouldTranslateHasWithPredicateArgValue() {
        final Bytecode bytecode = new Bytecode();
        bytecode.addStep("E");
        bytecode.addStep("has", "knows", "weight", P.eq(1.0));
        final Traversal.Admin<?, ?> translation = translator.translate(bytecode);
        assertEquals(g.E().has("knows", "weight", P.eq(1.0)).asAdmin(), translation);
    }

    @Test
    public void shouldTranslateHasWithNullThirdArgValue() {
        final Bytecode bytecode = new Bytecode();
        bytecode.addStep("E");
        bytecode.addStep("has", "knows", "weight", null);
        final Traversal.Admin<?, ?> translation = translator.translate(bytecode);
        assertEquals(g.E().has("knows", "weight", (String) null).asAdmin(), translation);
    }

}

[andreachild]

It looks like your IDE has auto style formatting which differs from the conventions used in the tinkerpop project and thus the wildcard import has been added and various whitespace changes as well. According to https://tinkerpop.apache.org/docs/current/dev/developer/#_code_style we try to keep the style consistent with the existing conventions.

Contributors should examine the current code base to determine what the code style patterns are and should match their style to what is already present. Of specific note however, TinkerPop does not use "import wildcards" - IDEs should be adjusted accordingly to not auto-wildcard the imports.

[andreachild]

I'm surprised the has overloads with 2 method args have not also surfaced also a problem (it could just be by luck depending on the order of the methods loaded in the global cache). You could change the logic to address both the 2 and 3 arg method overloads:

Suggested change

	} else if (newArguments.length == 3 && newArguments[2] == null && parametersTypes[0] == String.class &&
	parametersTypes[1] == String.class &&
	parametersTypes[2] == P.class) {
	//the second case has(String, String, (P)(null)) instead of has(String,String, (Object)null)
	found = false;
	}
	} else if (newArguments[newArguments.length - 1] == null && parametersTypes[newArguments.length - 1] == P.class) {
	//the second case has(String, String, (P)(null)) instead of has(String,String, (Object)null)
	//or has(String, (P)(null)) instead of has(String, (Object)null)
	found = false;
	}

[andrii0lomakin]

Hi.
I will reimplement this fix.

The correct fix is to sort the list of methods during the building method cache by isAssigned from the relationship. That will fix all current and future problems.

[andreachild]

Hi @andrii0lomakin I opened #3278 because I wanted a fix for the issue you found in 3.8.0 for which the release vote is happening today which unfortunately doesn't give you enough time to refactor the change as you mentioned. Please do continue with this PR and it can be added to 3.8.1 or 4.0. Thanks!