[jboss-jira] [JBoss JIRA] (AS7-6257) AttributeDefinition validateOperation should convert all expression strings to ModelType.EXPRESSION

[Brian Stansberry (JIRA)]

    [ https://issues.jboss.org/browse/AS7-6257?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12745672#comment-12745672 ] 

Brian Stansberry commented on AS7-6257:
---------------------------------------

An IRC discussion of pros and cons of this, confirming the decision to defer to 7.3:

bstansberry: maeste, kkhan, ctomc: re the general problem of expression strings not getting converted to ModelType.EXPRESSION...
[09:14am] bstansberry: AS7-6257
[09:14am] jbossbot: jira [AS7-6257] AttributeDefinition validateOperation should convert all expression strings to ModelType.EXPRESSION [Open (Unresolved) Enhancement, Major, Unassigned] https://issues.jboss.org/browse/AS7-6257
[09:14am] bstansberry: I'm tempted to fix that in 7.2
[09:14am] bstansberry: I scheduled it for 7.3 partly due to time constraints
[09:15am] bstansberry: now somewhat loosened
[09:15am] kkhan: bstansberry: Isn't that really a job for the parser? Then again I know CLI handles this somehow, but not sure about the console
[09:15am] ctomc: i dont think it would break that much if at all
[09:15am] bstansberry: and partly due to really vageue compatibilty concerns
[09:15am] bstansberry: kkhan: the parser is inadequate
[09:15am] bstansberry: since mgmt ops can come from arbitrary clients
[09:15am] bstansberry: CLI is inadequate as well for the same reason
[09:16am] bstansberry: this is why we convert in validateOperation as well
[09:16am] kkhan: It makes sense, but what about the corner case where someone **wants** a string of the form ${blah} for who knows what reason 
[09:17am] bstansberry: kkhan: yeah, that's my vague concern
[09:17am] ctomc: if attribute support expression than this string would be threated as one
[09:17am] ctomc: otherwise it would be plain string
[09:17am] bstansberry: ctomc: right but that JIRA suggests changing that
[09:17am] bstansberry: the current behavior is what you describe
[09:18am] kkhan: ${thing:${x}} 
[09:18am] kkhan: Sorry, I'm probably not being helpful
[09:19am] ctomc: hmm bstansberry what would we gain if we convert always?
[09:19am] ctomc: given that current impl works properly and convets only strings that are on attributes supporting expressions
[09:19am] bstansberry: ctomc: basically if people put expressions in string attributes that don't support it, we'll give a proper failure instead of some random failure at runtime
[09:20am] ctomc: ok, that does not mean we need to convert everything to expression
[09:20am] ctomc: we just convert for validation purposes not neccesary also store that in the model
[09:22am] bstansberry: ctomc: it won't validate though, so the end result is the same
[09:23am] ctomc: why wouldn't it validate?
[09:23am] ctomc: validateOperation would throw operationFailedException
[09:23am] bstansberry: right, that's what i mean -- it won't pass
[09:24am] ctomc: in case that value passed in looks like expression but shouldnt be
[09:24am] ctomc: is that not the outcome you would want?
[09:24am] ctomc: to report problem early?
[09:24am] bstansberry: I'm talking about:
[09:24am] bstansberry: ctomc: we just convert for validation purposes not neccesary also store that in the model
[09:25am] bstansberry: it won't pass validation so whether we store STRING or EXPRESSION in the model won't matter, because we won't store at all
[09:27am] kkhan: bstansberry: In light of what you are talking about, do you want: https://github.com/jbossas/jboss-as/pull/3876? I'm asking because if you do, perhaps it should be included in maeste's
[09:27am] jbossbot: git pull req [jboss-as] (open) kabir Scan for expressions in string model nodes https://github.com/jbossas/jboss-as/pull/3876
[09:27am] bstansberry: kkhan: yes, we definitely want that
[09:28am] kkhan: I'll ask maeste to include that then
[09:28am] bstansberry: I can't imagine we'll have tests where we *want* a STRING with ${} syntax
[09:29am] bstansberry: maybe some wild end-user use case would want it, but our testsuite doesn't test those 
[09:29am] kkhan: bstansberry: If we do, we can add a way to turn it off for a particular test
[09:29am] bstansberry: kkhan: exactly
[09:30am] ctomc: for cases like that more elegant solution is to have expression pointing to sys prop and that sys prop has ${} in it
[09:31am] bstansberry: re AS7-6257, we've talked about it; that was my goal. I'll paste ^^^ in the JIRA. Won't do for 7.2 unless one of you guys starts to think it's a great idea
[09:31am] jbossbot: jira [AS7-6257] AttributeDefinition validateOperation should convert all expression strings to ModelType.EXPRESSION [Open (Unresolved) Enhancement, Major, Unassigned] https://issues.jboss.org/browse/AS7-6257
[09:32am] ctomc: bstansberry: i have mixed feelings about this now, before we talked about it it sounded great 
                
> AttributeDefinition validateOperation should convert all expression strings to ModelType.EXPRESSION
> ---------------------------------------------------------------------------------------------------
>
>                 Key: AS7-6257
>                 URL: https://issues.jboss.org/browse/AS7-6257
>             Project: Application Server 7
>          Issue Type: Enhancement
>          Components: Domain Management
>            Reporter: Brian Stansberry
>             Fix For: 7.3.0.Alpha1
>
>
> AttributeDefinition.validateOperation currently only converts a string to an expression if the attribute supports expressions. It should convert any time it sees the syntax. This way if a STRING attribute that doesn't support expressions finds one, it will reject it instead of accepting it as a raw string.
> Scheduling for 7.3 out of concern this may break things in 7.2 without time to notice/adapt.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira