Convert Lighty to JAXRS components

[Tobianas]

This change also required us to convert LightyServerBuilder to use JettyWebServer since it is used in CommunityRestConf to initialize JaxRsEndpoint.

[Tobianas]

26.3. - Rebased and fixing openapi

[Tobianas]

OpenApi fixed, now fixing Http2 and Https

[gemini-code-assist]

Code Review

This pull request undertakes a significant refactoring to convert CommunityRestConf to use JaxRsEndpoint and transitions LightyServerBuilder to a new LightyJettyServerProvider that utilizes JettyWebServer from org.opendaylight.aaa.web.jetty. This aligns the server setup more closely with ODL's AAA web infrastructure, which is a positive direction.

However, there are some critical and medium severity concerns that need addressing, particularly the use of reflection to access private fields in JettyWebServer and the handling of WebContextSecurer for OpenAPI endpoints.

Summary of Findings

• Critical: Reflection to Access Private Fields: Http2LightyServerBuilder and HttpsLightyServerBuilder use reflection to access a private server field in JettyWebServer. This is a highly fragile and problematic practice that breaks encapsulation and should be avoided. An alternative approach for configuring the Jetty server is necessary.
• Medium: Unclear Usage of contexts in LightyJettyServerProvider: The LightyJettyServerProvider class has a contexts list populated by addContextHandler but doesn't seem to use this list to register contexts with the underlying JettyWebServer. Its purpose is unclear.
• Medium: Default No-Authentication for OpenAPI via null WebContextSecurer: Several modules and examples instantiate OpenApiLighty with a null WebContextSecurer, which defaults to a securer that disables authentication. This default behavior should be intentional and well-understood.
• Low: Default LightyWebContextSecurer Behavior (Not Commented): The default LightyWebContextSecurer in CommunityRestConf (and used by OpenApiLighty if no securer is provided) has empty requireAuthentication methods, effectively disabling authentication. The comment //do nothing since shiro is not used is brief; more explanation on implications could be useful.
• Low: Potential Typo in Method Name (Not Commented): The method getjaxRsOpenApi() in OpenApiLighty.java appears to have a typo and might be intended as getJaxRsOpenApi() or getOpenApiService().
• Low: Redundant Server Build in Test (Not Commented): In LightyJettyServerProviderTest.testServerBuilder, serverBuilder.build() is called redundantly before the main test logic that also builds the server.

Merge Readiness

This pull request is a work-in-progress (as indicated by the WIP title) and introduces significant architectural changes by adopting JettyWebServer and JaxRsEndpoint. While the direction is promising, there are critical issues, primarily the use of reflection in Http2LightyServerBuilder and HttpsLightyServerBuilder, that must be addressed before this PR can be considered for merging. Additionally, there are medium-severity concerns regarding the clarity of LightyJettyServerProvider.contexts usage and the default authentication behavior for OpenAPI endpoints that should also be reviewed.

I am unable to approve pull requests. It is recommended that these changes, especially the critical ones, are resolved and then further reviewed by other maintainers before merging.

[Tobianas]

/gemini review

[gemini-code-assist]

Code Review

This pull request undertakes a significant and beneficial refactoring by converting CommunityRestConf and related components to use the JaxRsEndpoint and ODL AAA's WebServer abstraction. This modernizes the Jetty integration, improves modularity, and aligns better with OpenDaylight's ecosystem. The introduction of LightyJettyWebServer and LightyJettyServerProvider is a key part of this.

While the direction is excellent, there are a few areas that could be refined, particularly around error handling and redundancy, as detailed in the comments. Given the "WIP" nature, these are likely points for further development.

Summary of Findings

• Error Handling in OpenApiLighty Initialization: If registerWebContext fails during OpenApiLighty initialization, the error is logged, but the module might appear to initialize successfully. This can lead to a NullPointerException during shutdown. The initialization should fail more explicitly.
• Exception Wrapping: In several places (AAALightyShiroProvider, LightyJettyServerProvider), ServletException is wrapped in a generic RuntimeException. This might obscure the original error type and could be reconsidered for better error diagnosis.
• Redundant Null Checks in Server Builders: Http2LightyServerBuilder and HttpsLightyServerBuilder contain redundant null checks for the server field in their build() methods, as the server is already initialized in their constructors.
• Architectural Improvement: The shift to using ODL AAA's WebServer abstraction and JaxRsEndpoint is a positive architectural change, promoting better modularity and integration.

Merge Readiness

This pull request is marked as 'WIP' and introduces substantial refactoring towards a more modern and modular web server integration. The changes are generally in a good direction. However, there's a high-severity issue in OpenApiLighty's error handling during initialization that needs to be addressed. Additionally, a few medium-severity points regarding exception wrapping and code redundancy should be considered.

Given the 'WIP' status and the identified issues (especially the high-severity one), I recommend addressing these points before merging. As an AI, I am not authorized to approve pull requests; please ensure further review and approval by authorized team members.

[gemini-code-assist]

Wrapping ServletException in a generic RuntimeException can obscure the original error type for callers. Could a more specific unchecked exception be more appropriate here, or should initAAAonServer (and subsequently the constructor) declare throws ServletException if it's a recoverable or specifically handleable error at a higher level? If this is considered a fatal initialization error, RuntimeException might be acceptable, but it's worth considering the impact on error diagnosis and handling.

[gemini-code-assist]

When a ServletException occurs during registerWebContext, it's caught and re-thrown as a generic RuntimeException. While this makes the build() method not declare ServletException, it might hide the specific nature of the problem from callers. Would it be more informative to either declare throws ServletException or use a custom, more specific unchecked exception that wraps the ServletException?

[Tobianas]

/gemini review

[gemini-code-assist]

Code Review

This pull request undertakes a significant and largely positive refactoring to convert CommunityRestConf to use JaxRsEndpoint and to overhaul the Jetty server setup by introducing LightyJettyWebServer and LightyJettyServerProvider. This modernizes the server interaction, promoting better modularity and adherence to the WebServer interface for component registration (like AAA and OpenAPI).

The changes are extensive and touch upon core server initialization, AAA, OpenAPI, and RestConf modules. The overall direction appears solid. However, there's a critical issue in how Http2LightyServerBuilder and HttpsLightyServerBuilder are updated, potentially leading to unintended behavior with server listeners.

Given the 'WIP' status, this feedback aims to highlight key areas for review and refinement.

Summary of Findings

• Potential unintended HTTP listener in server builders: Http2LightyServerBuilder and HttpsLightyServerBuilder re-initialize this.server with new LightyJettyWebServer(). This constructor path for LightyJettyWebServer sets up an HTTP connector on a random port. Consequently, the specialized builders might produce servers that listen on both the intended secure port and an additional random HTTP port. This was commented on with 'high' severity.
• Refactoring of Server Infrastructure: The core refactoring introduces LightyJettyWebServer (implementing WebServer) and LightyJettyServerProvider, replacing the older LightyServerBuilder paradigm. This is a positive change for modularity, allowing components like RestConf, AAA, and OpenAPI to register themselves via the WebServer interface. This was not commented on as it's the main goal of the PR and generally well-executed, aside from the server builder issue.
• AAA Module Refactoring: The AAALightyShiroProvider has been significantly refactored to use WebInitializer for setting up its servlets and filters, which is a cleaner approach than direct Jetty API manipulation. This was not commented on as it appears to be an improvement.
• OpenAPI Module Refactoring: OpenApiLighty now uses WebContext.builder() and WebServer.registerWebContext() for its setup, aligning with the new server infrastructure. This was not commented on as it's a positive change.
• CommunityRestConf to JaxRsEndpoint Conversion: CommunityRestConf now leverages JaxRsEndpoint, which itself uses the WebServer interface for registration. This is the central theme of the PR and seems to be implemented correctly by removing direct Jetty servlet setup from CommunityRestConf. This was not commented on as it's the primary goal.

Merge Readiness

This pull request is marked as 'WIP' and introduces substantial refactoring to the server initialization and component registration logic. While the overall direction towards using JaxRsEndpoint and a WebServer abstraction is commendable, there are high-severity issues in Http2LightyServerBuilder and HttpsLightyServerBuilder that could lead to unintended server behavior (extra HTTP listeners on random ports). These critical issues should be addressed before this PR is considered for merging. As an AI, I am not authorized to approve pull requests, but I recommend further review and resolution of the identified issues. Other parts of the refactoring appear to be well-structured.