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

Issue #4433 - Implement ServletContext.addJspFile #4463

Merged
merged 4 commits into from Jan 20, 2020

Conversation

janbartel
Copy link
Contributor

@janbartel janbartel commented Jan 8, 2020

Implement servlet 4.0 ServletContext.addJspFile.

Closes #4433

Signed-off-by: Jan Bartel <janb@webtide.com>
Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel janbartel added this to To Do in Jetty 10.0.0 via automation Jan 8, 2020
@janbartel janbartel added the Specification For all industry Specifications (IETF / Servlet / etc) label Jan 9, 2020
@janbartel janbartel requested a review from gregw January 9, 2020 00:01

root.addBean(new MySCIStarter(root.getServletContext(), new JSPAddingSCI()), true);
_server.start();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

All other tests in this class have code that actually make a HTTP request to test the functionality.
This new test only adds the dynamic JSP but does not test that making a HTTP request to it works.
Is that intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, should we not have 3 tests for this functionality? No mapping, partial mapping, existing mapping?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't add a test for a jsp file because jetty-servlet cannot depend on apache-jsp module. I've added an assertion that tests that the mapped jsp servlet is the one matched by the ServletHandler for the given path, that's as much as we can do.

I also added tests for an existing full mapping and an existing preliminary mapping.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a jspFile via this API in the test-spec webapp as an integration test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

great! can we then check that the test-distribution will see any fail for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Jetty 10.0.0 automation moved this from To Do to In Review Jan 9, 2020
@joakime joakime changed the title Jetty 10.0.x 4433 add jsp file Issue #4433 - Implement ServletContext.addJspFile Jan 9, 2020
Signed-off-by: Jan Bartel <janb@webtide.com>

root.addBean(new MySCIStarter(root.getServletContext(), new JSPAddingSCI()), true);
_server.start();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a jspFile via this API in the test-spec webapp as an integration test?

@janbartel janbartel requested a review from gregw January 17, 2020 07:27
Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel janbartel merged commit b520ca6 into jetty-10.0.x Jan 20, 2020
@janbartel janbartel deleted the jetty-10.0.x-4433-addJspFile branch January 20, 2020 08:45
@janbartel janbartel moved this from In Review to Done in Jetty 10.0.0 Jan 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Specification For all industry Specifications (IETF / Servlet / etc)
Projects
No open projects
Jetty 10.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

Implement ServletContext.addJspFile
3 participants