[3.0] Improve EclipseLink query parameter binding behavior

[dazey3]

for #1504

[lgtm-com]

This pull request fixes 36 alerts when merging be8c0b6 into cfda40a - view on LGTM.com

fixed alerts:

• 36 for Reference equality test of boxed types

[lgtm-com]

This pull request fixes 36 alerts when merging 7d0cc96 into cfda40a - view on LGTM.com

fixed alerts:

• 36 for Reference equality test of boxed types

[lukasj]

LGTM

[lukasj]

actually org.eclipse.persistence.testing.tests.queries.QueryFrameworkTestSuite$4.GetSQLTest test is failing with this change

[dazey3]

@lukasj

So, I'm a bit confused looking at the purpose of the test

eclipselink/foundation/eclipselink.core.test/src/it/java/org/eclipse/persistence/testing/tests/queries/QueryFrameworkTestSuite.java

Lines 432 to 449 in cfda40a

	public TestCase buildGetSQLTest() {
	TestCase test = new TestCase() {
	@Override
	public void test() {
	ReadAllQuery query = new ReadAllQuery(Employee.class);
	ExpressionBuilder builder = query.getExpressionBuilder();
	query.setSelectionCriteria(builder.get("firstName").equal(builder.getParameter("name")));
	query.addArgument("name");
	Record record = new DatabaseRecord();
	record.put("name", "Bob");
	String sql = query.getTranslatedSQLString(getSession(), record);
	if (sql.indexOf("?") != -1) {
	throwError("SQL was not translated.");
	}
	}
	};
	test.setName("GetSQLTest");
	return test;

The test seems to be asserting that if the translated query contains "?", then it was translated incorrectly.

Before this change:

    SELECT ID, FIRSTNAME, LASTNAME FROM employee WHERE (FIRSTNAME = 'Bob')

After this change:

    SELECT ID, FIRSTNAME, LASTNAME FROM employee WHERE (FIRSTNAME = ?)

Question:

1. Why does this test expect no parameter marker? The test is calling org.eclipse.persistence.expressions.Expression.getParameter(String parameterName) to create a ParameterExpression, but the test expects the translated SQL string to have a literal value? Is that what we should expect? I mean, org.eclipse.persistence.expressions.Expression.literal(String literal) exists...

If you agree with the new behavior I am delivering, I can change the test and add one for org.eclipse.persistence.expressions.Expression.literal(String literal), which doesn't exist. If you don't agree with the new behavior, read below.

---

Looking at the code changes I am making, I can see why this behavior has changed:

eclipselink/foundation/org.eclipse.persistence.core/src/main/java/org/eclipse/persistence/queries/DatabaseQuery.java

Lines 1499 to 1510 in cfda40a

	public String getTranslatedSQLString(org.eclipse.persistence.sessions.Session session, Record translationRow) {
	prepareCall(session, translationRow);
	// CR#2859559 fix to use Session and Record interfaces not impl classes.
	CallQueryMechanism queryMechanism = (CallQueryMechanism) getQueryMechanism();
	if (queryMechanism.getCall() == null) {
	return null;
	}
	SQLCall call = (SQLCall) queryMechanism.getCall().clone();
	call.setUsesBinding(false);
	call.translate((AbstractRecord) translationRow, queryMechanism.getModifyRow(), (AbstractSession) session);
	return call.getSQLString();
	}

The change that I am making is removing the arbitrary call.setUsesBinding(false);.

Question:

1. What is the purpose/usecase for this API call org.eclipse.persistence.queries.DatabaseQuery.getTranslatedSQLString()?

If this behavior change is unacceptable, I can put it back the way it is. In my opinion, it seems rather arbitrary to be disabling parameter binding here. But I did review the usage of getTranslatedSQLString() and it appears to only be used in testing.

[lgtm-com]

This pull request fixes 36 alerts when merging 5e598b2 into cfda40a - view on LGTM.com

fixed alerts:

• 36 for Reference equality test of boxed types

[lgtm-com]

This pull request fixes 36 alerts when merging fc539e4 into 85e3491 - view on LGTM.com

fixed alerts:

• 36 for Reference equality test of boxed types

[lgtm-com]

This pull request fixes 36 alerts when merging 72dc8a6 into 85e3491 - view on LGTM.com

fixed alerts:

• 36 for Reference equality test of boxed types

[dazey3]

@lukasj @rfelcman

It seems the build is failing to build DBWS:

[INFO] EclipseLink DBWS Builder ........................... FAILURE [  4.017 s]
...
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.2.0:jar (attach-javadocs) on project org.eclipse.persistence.dbws.builder: MavenReportException: Error while generating Javadoc: 
[ERROR] Exit code: 1 - /home/jenkins/agent/workspace/nk-pull-request-verifier_PR-1523/utils/org.eclipse.persistence.dbws.builder/src/main/java/org/eclipse/persistence/tools/dbws/BaseDBWSBuilderHelper.java:303: warning: no @param for operations
[ERROR]     public List<CompositeDatabaseType> buildTypesList(List<OperationModel> operations) {
[ERROR]                                        ^
[ERROR] /home/jenkins/agent/workspace/nk-pull-request-verifier_PR-1523/utils/org.eclipse.persistence.dbws.builder/src/main/java/org/eclipse/persistence/tools/dbws/BaseDBWSBuilderHelper.java:303: warning: no @return
[ERROR]     public List<CompositeDatabaseType> buildTypesList(List<OperationModel> operations) {
[ERROR]                                        ^
[ERROR] /home/jenkins/agent/workspace/nk-pull-request-verifier_PR-1523/utils/org.eclipse.persistence.dbws.builder/src/main/java/org/eclipse/persistence/tools/dbws/BaseDBWSBuilderHelper.java:356: warning: no @param for nct
[ERROR]     public void buildOROXProjects(NamingConventionTransformer nct) {
[ERROR]                 ^
[ERROR] /home/jenkins/agent/workspace/nk-pull-request-verifier_PR-1523/utils/org.eclipse.persistence.dbws.builder/src/main/java/org/eclipse/persistence/tools/dbws/BaseDBWSBuilderHelper.java:364: warning: no @param for nct

This doesn't seem to be related to my change. Is this a known issue?

[rfelcman]

@lukasj @rfelcman

It seems the build is failing to build DBWS:

[INFO] EclipseLink DBWS Builder ........................... FAILURE [  4.017 s]
...
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-javadoc-plugin:3.2.0:jar (attach-javadocs) on project org.eclipse.persistence.dbws.builder: MavenReportException: Error while generating Javadoc: 
[ERROR] Exit code: 1 - /home/jenkins/agent/workspace/nk-pull-request-verifier_PR-1523/utils/org.eclipse.persistence.dbws.builder/src/main/java/org/eclipse/persistence/tools/dbws/BaseDBWSBuilderHelper.java:303: warning: no @param for operations
[ERROR]     public List<CompositeDatabaseType> buildTypesList(List<OperationModel> operations) {
[ERROR]                                        ^
[ERROR] /home/jenkins/agent/workspace/nk-pull-request-verifier_PR-1523/utils/org.eclipse.persistence.dbws.builder/src/main/java/org/eclipse/persistence/tools/dbws/BaseDBWSBuilderHelper.java:303: warning: no @return
[ERROR]     public List<CompositeDatabaseType> buildTypesList(List<OperationModel> operations) {
[ERROR]                                        ^
[ERROR] /home/jenkins/agent/workspace/nk-pull-request-verifier_PR-1523/utils/org.eclipse.persistence.dbws.builder/src/main/java/org/eclipse/persistence/tools/dbws/BaseDBWSBuilderHelper.java:356: warning: no @param for nct
[ERROR]     public void buildOROXProjects(NamingConventionTransformer nct) {
[ERROR]                 ^
[ERROR] /home/jenkins/agent/workspace/nk-pull-request-verifier_PR-1523/utils/org.eclipse.persistence.dbws.builder/src/main/java/org/eclipse/persistence/tools/dbws/BaseDBWSBuilderHelper.java:364: warning: no @param for nct

This doesn't seem to be related to my change. Is this a known issue?

Hm Javadoc
Error: /home/runner/work/eclipselink/eclipselink/utils/org.eclipse.persistence.dbws.builder/src/main/java/org/eclipse/persistence/tools/dbws/XmlEntityMappingsGenerator.java:642: error: element not closed: ul Error: * <ul> Error: ^
Unclosed <li> insided?

[dazey3]

@rfelcman

Hm Javadoc
Error: /home/runner/work/eclipselink/eclipselink/utils/org.eclipse.persistence.dbws.builder/src/main/java/org/eclipse/persistence/tools/dbws/XmlEntityMappingsGenerator.java:642: error: element not closed: ul Error: *

Error: ^
Unclosed
• insided?

It's written the same way in master and I'm not seeing the same failures on master. Granted, the missing </ul> is missing here on 3.0 and not on master, so I fixed that. But if </li> is required, why is master not failing the same way?

[lgtm-com]

This pull request fixes 36 alerts when merging 1c8790a into 85e3491 - view on LGTM.com

fixed alerts:

• 36 for Reference equality test of boxed types

[dazey3]

@lukasj @rfelcman

Looks like the addition of the missing </ul> resolved the DBWS failures and it looks good now. I merged the changes back into 2.7 as well.