Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 358545 - Formatting a JSP file, containing Java snippets, leads to misformatted or even lost code
Summary: Formatting a JSP file, containing Java snippets, leads to misformatted or eve...
Status: VERIFIED FIXED
Alias: None
Product: WTP Source Editing
Classification: WebTools
Component: jst.jsp (show other bugs)
Version: 3.3.1   Edit
Hardware: PC Windows XP
: P3 critical with 1 vote (vote)
Target Milestone: 3.2.5   Edit
Assignee: Nitin Dahyabhai CLA
QA Contact: Nick Sandonato CLA
URL:
Whiteboard: PMC_approved, WI81015
Keywords:
Depends on: 361606
Blocks:
  Show dependency tree
 
Reported: 2011-09-22 05:24 EDT by Nicholas Wright CLA
Modified: 2011-10-23 18:16 EDT (History)
4 users (show)

See Also:
thatnitind: pmc_approved? (david_williams)
raghunathan.srinivasan: pmc_approved+
thatnitind: pmc_approved? (naci.dai)
thatnitind: pmc_approved? (deboer)
neil.hauge: pmc_approved+
thatnitind: pmc_approved? (kaloyan)
cbridgha: pmc_approved+
nsand.dev: review+


Attachments
When Format is run, code is lost and badly formatted to cause errors. (2.81 KB, text/plain)
2011-09-22 05:25 EDT, Nicholas Wright CLA
no flags Details
When Formatted, a small piece of code is formatted so it errors, but no code is actually removed. (506 bytes, text/plain)
2011-09-22 05:26 EDT, Nicholas Wright CLA
no flags Details
smaller test file (177 bytes, text/plain)
2011-10-17 18:39 EDT, Nitin Dahyabhai CLA
no flags Details
didn't generate properly (9.82 KB, patch)
2011-10-17 18:46 EDT, Nitin Dahyabhai CLA
no flags Details | Diff
patch 2 (40.01 KB, patch)
2011-10-18 15:18 EDT, Nitin Dahyabhai CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nicholas Wright CLA 2011-09-22 05:24:58 EDT
I have a Dynamic Web Project.  Not created from scratch, but with File Systems being imported.

A JSP file I had (BigBrokenJSP.jsp) would actually lose HTML/JSP code when I ran Format.  I didn't mind bad formatting, but losing HTML/JSP code is too dangerous for me to recommend Eclipse to my colleagues.

A smaller example (SmallBrokenJSP.jsp) doesn't remove code, but Eclipse breaks up the line starting "MM_showHideLayers" which then gives the file 7 errors, and makes the JSP file unusable.

This small snippet of code was taken from the larger file.

Will attach files.


-- Configuration Details --
Product: Eclipse 1.4.0.20110609-1120 (org.eclipse.epp.package.jee.product)
Installed Features:
 org.eclipse.jdt 3.7.0.v20110520-0800-7z8gFchFMTdFYKuLqBLqRja9B15B
Comment 1 Nicholas Wright CLA 2011-09-22 05:25:46 EDT
Created attachment 203826 [details]
When Format is run, code is lost and badly formatted to cause errors.
Comment 2 Nicholas Wright CLA 2011-09-22 05:26:17 EDT
Created attachment 203827 [details]
When Formatted, a small piece of code is formatted so it errors, but no code is actually removed.
Comment 3 Nick Sandonato CLA 2011-09-22 14:48:06 EDT
FormattingStrategyJSDT appears to be the strategy applying the problem changes.
Comment 4 Nitin Dahyabhai CLA 2011-10-17 18:39:58 EDT
Created attachment 205375 [details]
smaller test file
Comment 5 Nitin Dahyabhai CLA 2011-10-17 18:46:55 EDT
Created attachment 205376 [details]
didn't generate properly

Running either the JSP-Java or Client-side JavaScript formatting alone doesn't cause an error, but running them together when there's overlap causes some contents to be lost.  The simplest fix I can see is to make sure the JSP-Java formatting constrains itself to only create TextEdits for the areas contained in the partition it's asked to format.  The Client-side JavaScript formatting already does this, and while it's a mess, a rewrite I tried doesn't change the behavior in a way that fixes this bug.

Unit tests attached to check for data loss after formatting, but some tests in the org.eclipse.jst.jsp.ui.tests.format.TestContentFormatter suite still need updates for the indentation changes that were made to formatting during 3.3, and it needs to be added back into the main JSP UI suite.

Many thanks for the original file,...cs94njw.
Comment 6 Nitin Dahyabhai CLA 2011-10-17 18:49:44 EDT
Opened bug 361195 for a lingering indentation issue.
Comment 7 Nitin Dahyabhai CLA 2011-10-18 15:18:35 EDT
Created attachment 205454 [details]
patch 2
Comment 8 Nick Sandonato CLA 2011-10-19 11:56:39 EDT
The SSE side of things looks good, though I noticed a weird exception. It only ever happened once, though.

org.eclipse.wst.sse.core.internal.NotImplementedException: intentionally not implemented
at org.eclipse.wst.sse.core.internal.text.MinimalDocument.getLineOfOffset(MinimalDocument.java:216)
at org.eclipse.wst.jsdt.web.core.javascript.JsTranslator.translateJSNode(JsTranslator.java:604)
at org.eclipse.wst.jsdt.web.core.javascript.JsTranslator.translate(JsTranslator.java:321)
at org.eclipse.wst.jsdt.web.core.javascript.JsTranslator.reset(JsTranslator.java:275)
at org.eclipse.wst.jsdt.web.core.javascript.JsTranslator.run(JsTranslator.java:736)
at org.eclipse.core.internal.jobs.Worker.run(Worker.java:54)
Comment 9 Nitin Dahyabhai CLA 2011-10-19 13:36:44 EDT
Yeah, that is weird.  Adding the explicit cancel() call during release() removed most of those reported traces, but I see that one occurrence pop up irregularly, as well.
Comment 10 Nitin Dahyabhai CLA 2011-10-19 13:50:15 EDT
Retargetting to 3.2.5, as it happens there as well.

*Explain why you believe this is a stop-ship defect.

Cuases loss of data.

*Is there a work-around? If so, why do you believe the work-around is insufficient?

No workaround other than to not have scriptlets in client-side scripting areas.

*How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added?

Manual, plus added and updated JUnits.

*Give a brief technical overview. Who has reviewed this fix?

When formatting a JSP, the Platform API invokes a corresponding slave formatter for each partition in the text document.  Each formatter applies its changes right then, but the formatter for JSP Java scriptlets was not constraining itself to only making changes in the partition it was given during that call.  The Client-side JavaScript formatter was also not reliably getting its translation in JSP files, leading to those sections not being formatted properly.

*What is the risk associated with this fix?

Low.  The JSP-Java formatter is now using the same range limiting options when calling the JDT formatter as the Client-side JavaScript formatter has been using when calling the JSDT formatter (which itself is a fork from the JDT one).  Also, the unit tests specifically look for lost content when formatting the reporter-supplied attachment 203826 [details], as well as with the prior formatting unit tests.
Comment 11 Chuck Bridgham CLA 2011-10-19 14:14:05 EDT
approve - sounds like a bad problem, and has good test coverage
Comment 12 Nitin Dahyabhai CLA 2011-10-20 10:45:58 EDT
Released.
Comment 13 Nitin Dahyabhai CLA 2011-10-20 10:48:22 EDT
Opened bug 361553 for 3.3.x/3.4.
Comment 14 Nitin Dahyabhai CLA 2011-10-23 18:16:50 EDT
Verified with M-3.2.5-20111021173028