[WIP] Added basic validation for EL expressions

[ggam]

Fixes #123 and partially #104 (the part we can make without more discussion). This makes a very simple check intented only for xxExpression attributes where we know for sure we expect an expression.

This is still a work in progress. Remaining stuff is:

• Wait for existing tests to pass
• Add a new test to assert behavior
• Make the deployment fail if the expressions are invalid
• Create actual expressions to verify correctness instead of just checking the beginning and end of the string.

[wmhopkins]

@ggam -- I think this is a nice start -- I like the design -- but I'm not sure it's ready to submit yet. For one thing, it's only checking the XxxExpression() methods, but there are lots of other string-typed methods that can contain an EL expression and should be checked as well (with the difference that not being an EL expression isn't necessarily an error -- but if there is an EL expression there, it should be validated).

[ggam]

I guess I talked about this privately with @arjantijms. I'm only checking for those properties as they are the ones we can now for sure are EL expressions.

Users might put a #{foo........bar} literal value on a string property, whithout meaning to create an expression. We should not assume it's an expression and fail the deployment based on that. But, that said... the way it now works, Soteria will still assume it's an expression and will try to evaluate at runtime.

I'm not sure how we should handle that though. Expression prperties are clear as they can only contain EL expressions. For the other ones, we could:

• Try to create an expression if it seems to be it (starts with #{ and ends with }).
• If the previous assertion failed, assume it's not an expression and use its literal value.

Of course, we may confuse users by not giving them a clear error like "you put an invalid expression there", but would be safer than saying "I think you tried to put an expression here, but you make it wrong".

What do you think @wmhopkins @arjantijms @rdebusscher?

[wmhopkins]

I guess I'm not convinced you'd ever see ${...} or #{...} as a string-literal config value for anything, so I'm less concerned about accidentally failing a string that looks like EL, but isn't. I could be wrong about that, but, as you point out, the runtime is going to treat all strings that look like that as if they are EL expressions, regardless of whether we validate them or not.

Either way, the problem I have with submitting the current PR is that it will result in different behavior for validating EL expressions, depending whether it's an Expression field or not. From the user's point of view, some of their expressions will be validated, and others won't. It's inconsistent, regardless of the reason why. It seems better, to me, to not validate consistently, than to validate some and not validate others. There's no functional difference between the Expression fields and the non-Expression, so there's no reason to validate them differently, and it will seem arbitrary to users/developers. The only reason we're treating them differently is because the name gives us a clue about the content.

We should probably wait until we can validate them more accurately, without reliance on the name of the field.

[ggam]

We could validate all attributes in the same way, assumming the pitfall that a #{} literal will fail. But users will always have the workaround of using #{'#{string_literal}'} which should be good enough for now.

Also we could wait to have more consensus on validation, but then we are permitting more errors go to runtime. Since this is not the spec, and we can rectify wrong decissions if time demonstrates we were wrong, I'd just go for validating all the parameters if you agree.

If people complain about false-positives, we can always revisit this later. I know I'm very insistint with all this deployment-time validations, but as a day-to-day Java EE user, the real benefit I see about the deployment process is that it warns be about errors before my users face them.

Still would be good to hear some more people opinions. Maybe there are more important things to look at now.

[wmhopkins]

@ggam -- Closing this PR as we won't take it for the 1.0 release. Hopefully we can do something with broader support for string-based attributes that could be EL in the next release.