Issue #74 - remove trimmed servlets logic from Jetty12 EE8 & EE10 runtimes

[lachlan-roberts]

closes #74

I tried to maintain the existing logic of instantiating the servlets/filters/listeners and setting the asyncSupported while removing the rest of the TrimmedServlet/TrimmedFilter logic which is no longer necessary.

[code-review-assist]

Hi @lachlan-roberts, reviewers, and future viewers!

I'm Gemini, and I'm currently reviewing this pull request. In the meantime, here's a quick summary to get everyone up to speed:

This pull request addresses issue #74 by removing outdated servlet logic from the Jetty12 EE8 and EE10 runtimes. The primary goal is to streamline the codebase and remove unnecessary components. The author, @lachlan-roberts, aimed to maintain the core functionality of servlet/filter/listener instantiation and asyncSupported settings while eliminating the now-obsolete TrimmedServlet/TrimmedFilter logic.

Here's a breakdown of the changes:

• runtime/runtime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/ee10/AppEngineWebAppContext.java: Significant modifications were made to this file. Lines 16-47 show the removal of several import statements related to deprecated servlets and filters. The large diff starting at line 77 removes a large block of code (lines 94-116) related to deprecated servlets and filters. Further changes on lines 171-191 simplify the addEventListener and removeEventListener methods. Finally, the major change starting at line 200 removes the TrimmedServlets and TrimmedFilters logic, replacing it with methods to directly instantiate Jetty servlets, filters, and listeners (lines 228-282). The HolderMatcher class (lines 405-444) and related classes are also removed.
• runtime/runtime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/ee8/AppEngineWebAppContext.java: Similar changes were made to the EE8 counterpart, mirroring the structure of the EE10 changes. The deprecated servlet and filter logic was removed, and the instantiation of Jetty components was streamlined. The HolderMatcher class and related classes are also removed.

I'll provide a more detailed review shortly. In the meantime, please let me know if this summary is helpful or if you have any questions.

And now, a little haiku to brighten your day:

Code flows like a stream,
Cleansed of old, unused parts,
New clarity shines bright.

[code-review-assist]

This pull request correctly addresses issue #74 by removing the unnecessary TrimmedServlet and TrimmedFilter logic. The approach of maintaining the existing instantiation logic for servlets, filters, and listeners while removing the trimming logic is sound. Setting the asyncSupported property directly on the holders is a cleaner approach than relying on the older XML configuration. The introduction of the instantiateJetty* methods is a good way to ensure that Jetty components are instantiated correctly. The changes are well-structured and improve the maintainability of the codebase. The commit message clearly explains the changes and their purpose.

Here's a summary of the referenced elements from the Google Java Style Guide:

• Whitespace: Consistent use of 4 spaces for indentation.
• Naming: Clear and concise names for methods and variables.
• Javadoc: Adequate Javadoc for new methods.

[code-review-assist]

Hi @lachlan-roberts, reviewers, and future viewers,

I'm currently reviewing this pull request and will post my full review in a few minutes. In the meantime, here's a quick summary to help everyone get up to speed:

This pull request addresses issue #74 by removing the TrimmedServlets and TrimmedFilters logic from the Jetty12 EE8 and EE10 runtimes. The author, @lachlan-roberts, states that this logic is no longer necessary.

Here's a breakdown of the changes:

• runtime/runtime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/ee10/AppEngineWebAppContext.java: This file underwent significant changes. The DEPRECATED_SERVLETS_FILTERS constant (lines 94-116 in the before diff) and the related TrimmedServlets and TrimmedFilters classes (lines 405-690 in the before diff) have been removed. The logic to instantiate Jetty servlets, filters, and listeners has been refactored and simplified (lines 200-282 in the after diff). The addEventListener and removeEventListener methods have been slightly cleaned up (lines 171-191 in the after diff). The startWebapp method now sets AsyncSupported for servlets and filters (lines 206-211 in the after diff).
• runtime/runtime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/ee8/AppEngineWebAppContext.java: Similar changes were made to the EE8 version of the file, mirroring the changes in the EE10 version. The deprecated filters and servlets logic was removed, and the instantiation of Jetty components was refactored (lines 198-316 in the after diff).

The overall intent is to streamline the codebase and remove obsolete functionality. The changes appear to be well-structured and maintain the existing functionality while simplifying the code.

---

Here's a little haiku to lighten the mood:

Code flows like a stream,
Removing old, unused parts,
Clean code shines anew.

[code-review-assist]

Gemini encountered an error creating the review. You can try again by commenting @code-review-assist review.

[code-review-assist]

Hi @lachlan-roberts and reviewers,

I'm currently reviewing this pull request and will post a full review in a few minutes. In the meantime, here's a summary to help everyone quickly get up to speed:

This pull request addresses issue #74 by removing outdated servlet logic from the Jetty12 EE8 and EE10 runtimes. The primary goal is to streamline the codebase and remove unnecessary components.

Here's a breakdown of the changes:

• runtime/runtime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/ee10/AppEngineWebAppContext.java: This file underwent significant changes. The major changes include:
  • Lines 16-47: Removed several import statements for deprecated servlets and filters (lines 22-27 in the before diff). Added several new imports (lines 19-40 in the after diff).
  • Lines 77-80: Removed a large block of code (lines 94-116 in the before diff) related to deprecated servlets and filters.
  • Lines 171-190: Minor formatting changes to improve code readability (lines 209-229 in the before diff).
  • Lines 200-282: The startWebapp method was heavily revised. The old logic using TrimmedServlets and TrimmedFilters was removed (lines 238-275 in the before diff). New methods instantiateJettyServlets, instantiateJettyFilters, and instantiateJettyListeners were added to handle instantiation of Jetty components. The setAsyncSupported method is now called for all servlets and filters (lines 206-212 in the after diff).
  • Lines 308-310: Removed the instantiateJettyListeners method (lines 317-332 in the before diff) as its functionality is now integrated into startWebapp.
  • Lines 378-381: Removed the HolderMatcher, TrimmedServlets, and TrimmedFilters classes (lines 405-699 in the before diff).
• runtime/runtime_impl_jetty12/src/main/java/com/google/apphosting/runtime/jetty/ee8/AppEngineWebAppContext.java: Similar changes were made to this file, mirroring the modifications in the EE10 version. The deprecated servlet and filter logic was removed, and the code was restructured to use the new instantiation methods.

The author mentions maintaining the existing logic for instantiating servlets, filters, and listeners while removing the trimmed servlet/filter logic. The changes reflect this intent.

Let me know if you have any questions.

And now, a little haiku to brighten your day:

Code flows like a stream,
Cleaning up, a smoother run,
Bugs flee in the night.

[code-review-assist]

This pull request removes the trimmed servlets logic and related classes, which are no longer necessary. It also sets the asyncSupported property for servlets and filters based on the APP_IS_ASYNC system property. The changes are well-structured and improve code clarity by removing obsolete logic. The commit message clearly explains the changes and references the related issue. I have a few minor suggestions below, mostly related to code style and documentation.

It's good to see the removal of obsolete logic related to handling deprecated filters and servlets. The new approach of setting asyncSupported directly is cleaner and more efficient. The introduction of helper methods like instantiateJettyServlets, instantiateJettyFilters, and instantiateJettyListeners improves code organization and readability. Overall, this is a well-crafted PR.

[lachlan-roberts]

There are some test failures.

Looks like we do require most of the logic in the TrimmedServlets / TrimmedFilters classes to ensure certain servlets/filters are created. So I think the only thing we no longer need is the ImmutableSet<HolderMatcher> DEPRECATED_SERVLETS_FILTERS.

[ludoch]

I think we can also remove this for Java11 and Java17 with Jetty9.4

Only the old java8 runtime need to keep this ancient behavior.

[lachlan-roberts]

@ludoch doesn't the java8 runtime use the same code path for this as java11-17?

[ludoch]

@ludoch doesn't the java8 runtime use the same code path for this as java11-17?

Yes, but we could also remove this in the java11-17 code path when the runtime is not java8

I see you currently changed only 2 path for jetty12, we could remove a bit more but not for java8, in runtime/runtime_impl_jetty9/src/main/java/com/google/apphosting/runtime/jetty9/AppEngineWebAppContext.java