Skip to content

[OGNL-102] Improves performance when null was returned#140

Closed
lukaszlenart wants to merge 1 commit intomasterfrom
OGNL-102-performance
Closed

[OGNL-102] Improves performance when null was returned#140
lukaszlenart wants to merge 1 commit intomasterfrom
OGNL-102-performance

Conversation

@lukaszlenart
Copy link
Copy Markdown
Collaborator

@lukaszlenart lukaszlenart commented Nov 23, 2021

Improves performance of chained expressions in case if the root is null

Refs OGNL-102

@lukaszlenart lukaszlenart force-pushed the OGNL-102-performance branch from 137eadc to 54b18de Compare March 5, 2022 09:56
@lukaszlenart lukaszlenart force-pushed the OGNL-102-performance branch from 54b18de to e5e2491 Compare March 5, 2022 09:58
@lukaszlenart
Copy link
Copy Markdown
Collaborator Author

@davoustp @harawata @quaff could you take a look? this should improve performance a bit, it's in relation to #138 and the JIRA ticket.

@harawata
Copy link
Copy Markdown
Contributor

Hi @lukaszlenart ,

I'm sorry about the slow response (as always)!

So, is root the only concern here?

With the following test, for example, the for-loop is still evaluated for the second node and the similar exception is thrown :

ognl.OgnlException: source is null for getProperty(null, "key2")

public void testNullHnadling() throws Exception {
  OgnlContext context = (OgnlContext) Ognl.createDefaultContext(null, new DefaultMemberAccess(false));
  Map<String, Object> root = new HashMap<>();
  root.put("key1", null);
  String expr = "key1.key2.key3";
  assertNull(Ognl.getValue(expr, context, root));
}

@lukaszlenart
Copy link
Copy Markdown
Collaborator Author

Right, but if I did that on children level in the for loop a lot of tests have started to fail, especially this one

@harawata
Copy link
Copy Markdown
Contributor

Yes, it will change the designed behavior and @davoustp seems to be aware.

I'll just show some tests that emphasizes the difference in behavior.
If these results match @davoustp 's expectation, this PR should be fine.

public void testNullValue() throws Exception {
  OgnlContext context = (OgnlContext) Ognl.createDefaultContext(null, new DefaultMemberAccess(false));
  Map<String, Object> root = new HashMap<>();
  root.put("key1", null);
  String expr = "key1.key2.key3";
  assertNull(Ognl.getValue(expr, context, root)); // FAIL : OgnlException
}

public void testEmptyRoot() throws Exception {
    OgnlContext context = (OgnlContext) Ognl.createDefaultContext(null, new DefaultMemberAccess(false));
    Map<String, Object> root = new HashMap<>();
    String expr = "key1.key2.key3";
    assertNull(Ognl.getValue(expr, context, root)); // FAIL : OgnlException
}

public void testNullRoot() throws Exception {
    OgnlContext context = (OgnlContext) Ognl.createDefaultContext(null, new DefaultMemberAccess(false));
    Map<String, Object> root = null;
    String expr = "key1.key2.key3";
    assertNull(Ognl.getValue(expr, context, root)); // PASS
}

@lukaszlenart
Copy link
Copy Markdown
Collaborator Author

Changes will be moved into #155

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants