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

Remove unused DOMParserWrapper and strip down PDEXMLHelper #665

Merged
merged 2 commits into from
Aug 13, 2023

Conversation

HannesWell
Copy link
Member

@jukzi you just worked in the area of XML processing recently, do you know any use-case of the to be removed classes?

@github-actions
Copy link

github-actions bot commented Jul 11, 2023

Test Results

     246 files  ±0       246 suites  ±0   1h 0m 20s ⏱️ + 4m 5s
  3 299 tests ±0    3 275 ✔️ +1  24 💤 ±0  0 ±0 
10 194 runs  ±0  10 122 ✔️ +1  72 💤 ±0  0 ±0 

Results for commit f56c9d7. ± Comparison against base commit e621405.

♻️ This comment has been updated with latest results.

@jukzi
Copy link
Contributor

jukzi commented Jul 12, 2023

@jukzi you just worked in the area of XML processing recently, do you know any use-case of the to be removed classes?

I am not familiar with the code. Only did technical change.

@HannesWell
Copy link
Member Author

Does anybody know why there was a XMLErrorReporter in the pde.ds.core that was in most parts a duplicate of the XMLErrorReporter in pde.core?
I assume that the code was just duplicated long ago and diverged until today since the duplicate was forgotten.

Nevertheless it should be checked if the differences are relevant. Does anybody know this already?

@SuppressWarnings("restriction")
SAXParser parser = org.eclipse.core.internal.runtime.XmlProcessorFactory
.createSAXParserWithErrorOnDOCTYPE();
parser.parse(stream, handler);

Check failure

Code scanning / CodeQL

Resolving XML external entity in user-controlled data Critical

XML parsing depends on a
user-provided value
without guarding against external entity expansion.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting: CodeQL added a false positive marker here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I assume it does not understand the method that creates the factory.

- Inline SAXParserWrapper
- Remove unused code from PDEXMLHelper
- Use XMLProcessorFactory from eclipse.platform in favor of a own one
@HannesWell HannesWell merged commit 7b86df0 into eclipse-pde:master Aug 13, 2023
12 of 13 checks passed
@HannesWell HannesWell deleted the removeUnusedXML branch August 13, 2023 21:56
@@ -84,10 +83,11 @@ public class XmlReferenceDescriptorWriter {
* to write the reports to
* @param debug if debugging infos should be written out to the console
*/
@SuppressWarnings("restriction")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using a local variable could move the suppress to the actual line of code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the parser field is also used below and since this method is relatively small in scope I skipped the extra variable in this case for simpler code.

@jukzi
Copy link
Contributor

jukzi commented Aug 14, 2023

looks like a lot of work. thanks.

@HannesWell HannesWell added this to the 4.29 M3 milestone Aug 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants