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.

Sign up for GitHub

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

Jump to bottom

Issue #4631 - Warning about skipping of <Arg> nodes is in wrong place for <Configure> #4632

Merged
merged 8 commits into from
Mar 10, 2020
Merged

Issue #4631 - Warning about skipping of <Arg> nodes is in wrong place for <Configure> #4632

merged 8 commits into from
Mar 10, 2020

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Mar 2, 2020

The warning for skipping of <Arg> nodes was left over after the work done in issue #4550
This warning isn't appropriate in the original place any more.

Added unit tests so that warnings are always in the appropriate place.

@joakime
Issue #4631 - Fixing XML comment that was accidentally reformatted
b839ce6 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Issue #4631 - Warning about skipping of <Arg> nodes is in wrong place…
cfdffd1 
… for <Configure>

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime requested a review from sbordet March 2, 2020 22:29
@joakime joakime self-assigned this Mar 2, 2020
@joakime joakime added the Bug For general bugs on Jetty side label Mar 2, 2020
@joakime joakime linked an issue Mar 2, 2020 that may be closed by this pull request
Startup XmlConfiguration WARN on Arg threadpool #4631
Closed
@joakime joakime mentioned this pull request Mar 2, 2020
Closed
@joakime
Issue #4631 - Improving testcase
89f2cad 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
sbordet
sbordet requested changes Mar 3, 2020
View reviewed changes
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

Do we really need the Bogus* classes? They don't add anything and I would rather use existing test classes that have similar signatures to the Bogus* introduced.

@joakime
Copy link
Contributor Author

joakime commented Mar 3, 2020 •
edited
Loading

They are just test classes to use during testing of XmlConfiguration.
Starting them with "Test" would have triggered both junit and surefire to treat them as test cases expecting a lot of other things that are not relevant.
jetty-xml only depends on jetty-util, so using the real classes isn't an option.
Besides, the extra requirements of the real classes just would have made the tests far too noisy for no good reason.
The existing test classes in jetty-xml have grown to be ridiculous and are super fragile now.

@joakime
Issue #4631 - Removing test classes
3ee3246 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Issue #4631 - Cleaning up configure with index per PR review
da7e8ed 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Issue #4631 - More named arg test cases
69fa180 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime requested a review from sbordet March 3, 2020 17:43
@joakime
Issue #4631 - Add testConfiguredWithNamedArgNotFirst
cc80ebe 
+ new testcase where <Arg> is needed, but is not the first node

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@gregw gregw self-requested a review March 9, 2020 09:34
gregw
gregw requested changes Mar 9, 2020
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Something has gone wrong before this PR... but I think it might simplify this PR to deal with it (see comment below).

However I have tested the branch and this impl works fine.

jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java Show resolved Hide resolved
jetty-xml/src/main/java/org/eclipse/jetty/xml/XmlConfiguration.java Show resolved Hide resolved
@gregw
Copy link
Contributor

gregw commented Mar 10, 2020

@joakime I suspect it was me that broke the index mechanism some time ago.... I'll see if I can fix it for this PR.... stand by....

@gregw
Cleanup configuration index usage
ad3149c 
Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw merged commit 965483e into jetty-9.4.x Mar 10, 2020
sbordet
sbordet requested changes Mar 10, 2020
View reviewed changes
jetty-xml/src/test/java/org/eclipse/jetty/xml/XmlConfigurationTest.java
last = configuration;
}
return last.getIdMap();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to see a test where there is a Arg misplaced, as below, so that we define in a test how we should behave.

<Configure ...>
  <Arg>...</Arg>
  <New ...>...</New>
  <Arg>...</Arg>
</Configure>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gregw pointed out that the Configure_9_3.dtd (and Configure_10_0.dtd) does not allow a <Arg> anywhere but the start of the element list.

https://github.com/eclipse/jetty.project/blob/678385bfda0f90a0bcccf7f86aa272073c1b4f29/jetty-xml/src/main/resources/org/eclipse/jetty/xml/configure_10_0.dtd#L45-L46

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A test that is expected to fail in this scenario is dependent on the dtd used (or not used) I would think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then shouldn't we do the same for Call and New? I don't think the DTD has the same enforcement for those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened #4658 for these new test cases.

Also <Call>

https://github.com/eclipse/jetty.project/blob/678385bfda0f90a0bcccf7f86aa272073c1b4f29/jetty-xml/src/main/resources/org/eclipse/jetty/xml/configure_10_0.dtd#L168-L169

And <New>

https://github.com/eclipse/jetty.project/blob/678385bfda0f90a0bcccf7f86aa272073c1b4f29/jetty-xml/src/main/resources/org/eclipse/jetty/xml/configure_10_0.dtd#L209-L210

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also opened #4659 for the missing Id?,Name?,Class?,Arg* in the <Configure> section on Configure_10_0.dtd

Copy link
Contributor

Choose a reason for hiding this comment

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

@sbordet we had such a test, but that is invalid XML against the DTD. I don't think we should be required to process non-valid XML

Note that I'm doing a cleanup in 10 - see #4656

@joakime joakime mentioned this pull request Mar 10, 2020
Closed
@gregw gregw deleted the jetty-9.4.x-4631-xmlwarn-threadpool branch March 10, 2020 14:28
@joakime joakime mentioned this pull request Mar 10, 2020
Closed
@sbordet sbordet mentioned this pull request Mar 31, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet requested changes

@gregw gregw gregw approved these changes

Assignees

@joakime joakime

Labels
Bug For general bugs on Jetty side
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Startup XmlConfiguration WARN on Arg threadpool
3 participants
@joakime @gregw @sbordet