Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SpEL should not restrict 'NEW' identificator as 'new' token for constructor [SPR-11783] #16405

Closed
spring-issuemaster opened this issue May 13, 2014 · 9 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented May 13, 2014

Artem Bilan opened SPR-11783 and commented

See SO post.

It looks OK for other literal tokens as operators for SpEL, but NEW should not be interpreted as new operator.


Affects: 3.2.8, 4.0.4

Reference URL: http://stackoverflow.com/questions/23630933/spelparseexception-on-setting-key-of-messageheader-to-new-in-spring-integratio

Referenced from: commits c382b6f

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 15, 2014

Juergen Hoeller commented

Note that map['NEW'] works, just map[NEW] doesn't... Since the latter is essentially a shortcut for the fully demarcated former variant, it's arguably to be expected that it fails when keywords are used; that's why we have literal String demarcation. The same applies to map[T] versus map['T'], since even T is a keyword in SpEL (for referring to a Java type by name).

Fair enough, we could be smarter about identifying obvious cases where it's not meant as a keyword (independent from the casing) but that might not be as trivial as it seems. I'll turn this issue into a corresponding enhancement request.

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 15, 2014

Artem Bilan commented

Thank you, Juergen, for your attention and feedback!

However map['NEW'] doesn't work in case of NamedParameterJdbcTemplate.
The query shown in SO post:

<jdbc:outbound-gateway query="select name from emp where status=:headers[NEW]"/>

is parsed by NamedParameterUtils and it doesn't support quotes in the parameter names. With quotes we end up with sqlToUse as

select name from emp where status=?'NEW']

I won't mind to not represented it as a bug, because we have a lot of workarounds on the matter. And even won't mind, if we just document those restrictions.
As you say: it might not be so trivial to fix.

It is just an aditional info to understand the issue.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 15, 2014

Juergen Hoeller commented

Good point, if expressions are embedded in a format where quotes have a different meaning, this becomes a harder problem. Those quotes would need to be escaped in such a scenario, since SpEL itself isn't meant to be quote-free and therefore not designed for plain embedding into formats where quotes have a different meaning. Of course, we could re-define SpEL to allow for expressing everything without the use of quotes; I'm just not sure how feasible it is to aim for being 100% quote-free there.

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 28, 2014

Andy Clement commented

I've just had a look and allowing new/NEW or T/t without quotes as a map key is not too tricky. It is special casing in the parser but I don't think it complicates things too much. If you start using '.' in your identifiers we would continue, however, to require them to be quoted, you can't say somemap[abc.def] today, you must say somemap['abc.def'] and supporting new/t would be no different, you'd need to quote: somemap['new.key'].

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 25, 2015

Juergen Hoeller commented

Artem Bilan, Andy Clement, if we decide to do this, let's do it for 4.2 right away...

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 25, 2015

Artem Bilan commented

I won't mind either way.
For me it would be just enough do not treat NEW identifier as new operator.
As well as the same for T operator, but not for t identifier.

All other cases mentioned by Andy are valid and really should not be changed.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 30, 2015

Andy Clement commented

Changes committed to support this, with tests.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 31, 2015

Juergen Hoeller commented

Cool - thanks, Andy!

Let's see what Artem Bilan says once he had a chance to try it :-)

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 31, 2015

Artem Bilan commented

Thank you guys, looks really cool!

Nothing to complain about: we break nothing (as I expected) and add a feature to be patient for end-user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.