Bugzilla – Full Text Bug Listing
| Summary: | Multi-line constraints cause syntax error in generated validator class | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Modeling] EMF | Reporter: | Axel Uhl <eclipse> | ||||||||||||||||
| Component: | Tools | Assignee: | Kenn Hussey <Kenn.Hussey> | ||||||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||||||
| Severity: | normal | ||||||||||||||||||
| Priority: | P3 | CC: | eclipse, Ed.Merks, ed, Kenn.Hussey | ||||||||||||||||
| Version: | unspecified | Flags: | Kenn.Hussey:
helios+
Kenn.Hussey: pmc_approved+ Ed.Merks: pmc_approved+ Kenn.Hussey: review+ Ed.Merks: review+ |
||||||||||||||||
| Target Milestone: | RC4 | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | Windows XP | ||||||||||||||||||
| Whiteboard: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
Created attachment 170633 [details]
Generate a proper literal.
Using multi-line support with indentation in this context just doesn't make sense. A string literal is being produced and *all* special characters need escaping in this context. I think this must be changed before we release broken API.
Created attachment 170667 [details]
better patch
Newlines were already being handled correctly for invariant expressions, so this patch applies the same strategy for constraints. Neither were properly escaping literals, however (ad Ed discovered when creating his patch); this patch ensures that the necessary escaping is also done.
Axel, please confirm that this updated patch will meet your needs.
Created attachment 170753 [details]
Fixes the indent method regarding subsequent escape chars
*** Bug 315352 has been marked as a duplicate of this bug. *** (In reply to comment #4) > *** Bug 315352 has been marked as a duplicate of this bug. *** The duplicate has a much simpler patch. It's not the indent() method as such that is the problem. It's the indenting of multiple lines, so indent() can be left as-is. Literals.toStringLiteral() can make the constraint string safe to indent. Yes, it's simpler to simply escape all the characters to produce one large string---my original obsoleted patch did that---but if one has an expression with dozens of lines, each very long, then producing a single huge line is not as desirable as producing a literal per line. Note that with your patch, the call to indent is pointless given that the string can no longer contain actual \n or \r characters; they've been escaped. (In reply to comment #6) > Note that with your patch, the call to indent is pointless given that the > string can no longer contain actual \n or \r characters; they've been escaped. Indeed. My patch was as simple as possible to minimise RC4 risk. We can certainly do better; changing indent() might ripple... I'll try the new patch this evening. Created attachment 170797 [details]
final patch
Thanks, Axel, for the corrections. Here's a proposed "final" patch. If everyone is OK with this, I'll commit the changes ASAP.
The patch is functional for me (so a +1 for utility, but only +0.5 for accuracy). The Java literal omits the \n and \r characters so the literal does not accurately represent the Ecore content.
e.g. For an ValidationDelegate I get:
protected static final String MEMBER__AT_MOST_TWO_LOANS__EEXPRESSION = "" +
"\t\t" +
"\t\t" +
"\t\tloans->size() <= 2";
whereas in the annotations I get:
addAnnotation
(getBook_Loans(),
source,
new String[] {
"derivation", "\n\t\t\t\n\t\t\t\n\t\t\t\n\t\t\tlibrary.loans->\n select(book=self)"
});
addAnnotation
(memberEClass,
source,
new String[] {
"AtMostTwoLoans", "\n\t\t\n\t\t\n\t\tloans->size() <= 2",
"UniqueLoans", "\n\t\t\n\t\t\n\t\tloans->isUnique(book)" });
Pretty printing is nice but it should be accurate.
[The above unfortunately demonstrates that the xxx__EEXPRESSION was not actually required at all. The AtMostTwoLoans body is code generated in two different ways.]
I hadn't checked that, but it's obvious that this will lead to bugs. Consider OCL expressions that contain line comments introduced with "--" leading up to the end of the line. This means, line breaks *have* to be preserved in the string literal. Created attachment 170889 [details]
final final patch
OK, how about this?
Created attachment 170912 [details]
Fix for control problems
-1 to the "final final patch".
The increments needed to be 4:2 not 3:1 since the -1 shouldn't be done twice. (The trailing " of the indentation was being escaped.)
The attached patch separates the two concerns of
a) change the string
b) repair the loop counter
by using a to-go loop counter that doesn't need repair.
This eliminates the opportunity for the control logic error and makes the code a bit smaller and faster and simpler.
(In reply to comment #12) > The increments needed to be 4:2 not 3:1 since the -1 shouldn't be done twice. Yeah, I somehow forgot that the increment already accounted for the -1. > The attached patch separates the two concerns of > > a) change the string > b) repair the loop counter > > by using a to-go loop counter that doesn't need repair. Hmm. Doesn't it still need repair in the case that two characters are replaced instead of one? Given how different the alternative patch is, I'm inclined to just commit my latest patch, with the corrected indexes. Created attachment 170957 [details]
corrected final final patch :)
If there are no objections, I will commit this first thing tomorrow (Friday).
(In reply to comment #14) +1 (In reply to comment #13) > Hmm. Doesn't it still need repair in the case that two characters are replaced > instead of one? Yes. Oops. We have no JUnit test and I only exercised it with \n not \r\n. In place updates and loop counter modifications are always difficult; in combination as here they're fatal. We've both messed up. Sometimes I've created a local CharStream to eliminate the loop counter and allow multi-intra-loop-reads and passing to subroutines. Often I use a separate output buffer to avoid input/output interactions. I think we need at least a separate output buffer. The changes have been committed to CVS. This fix is available in 2.6.0 RC4. |
Build Identifier: 20100318-1801 When a constraint provided by a validator's annotation contains newlines, the ValidatorClass.javajet template generates uncompilable Java code. It may be helpful to replace newlines found in the annotation into \n escapes so as to generate compilable Java code. Below please find a possible patch that solves the problem in our case. Index: src/org/eclipse/emf/codegen/ecore/genmodel/impl/GenClassifierImpl.java =================================================================== --- src/org/eclipse/emf/codegen/ecore/genmodel/impl/GenClassifierImpl.java (revision 652) +++ src/org/eclipse/emf/codegen/ecore/genmodel/impl/GenClassifierImpl.java (working copy) @@ -325,7 +325,7 @@ public String getConstraintExpression(String constraint, String indentation) { - return indent(getConstraintExpression(constraint), indentation); + return indent(getConstraintExpression(constraint).replace('\r', ' ').replace("\n", "\\n"), indentation); } public String getValidationDelegate(String constraint) Reproducible: Always Steps to Reproduce: 1. Create an Ecore model with a class that has an OCL constraint which contains newlines 2. Create a genmodel off of it 3. Generate the model code and try to compile the validator classes in the util package