Allow Lighty to get restconf path from configuration#2241
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @Tobianas, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses a limitation where the RESTCONF root path was hardcoded within Lighty. It introduces the necessary changes to allow Lighty to read and apply the RESTCONF servlet context path from its configuration, providing greater flexibility for deployment environments and aligning with the original intent of supporting different endpoints as per JIRA LIGHTY-375.
Highlights
- Configurable RESTCONF Root Path: The previously hardcoded
/restconfroot element name for the RESTCONF API is now configurable, allowing Lighty to dynamically determine the path from its application configuration. - Dynamic Configuration Loading: The
RESTCONF_CONFIG_ROOT_ELEMENT_NAMEfield inRestConfConfigUtilsis now updated at runtime by reading therestconfServletContextPathproperty from therestconfsection within the loaded JSON configuration. - Field Modifiability: The
RESTCONF_CONFIG_ROOT_ELEMENT_NAMEfield's declaration was changed frompublic static final Stringtoprivate static String, enabling its value to be modified after initialization based on configuration.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces the ability to configure the RESTCONF root element name from a configuration file. The changes involve modifying a static variable and adding logic to parse the configuration file. I've identified potential NullPointerException and IndexOutOfBoundsException issues due to missing null checks and boundary checks, and also pointed out code duplication.
| return new RestConfConfiguration(); | ||
| } else { | ||
| RESTCONF_CONFIG_ROOT_ELEMENT_NAME = configNode.get("restconf").get( | ||
| "restconfServletContextPath").asText().substring(1); |
There was a problem hiding this comment.
The code assumes that restconfServletContextPath always starts with a /. If it doesn't, substring(1) will cause an IndexOutOfBoundsException. Add a check to ensure the string is not empty and starts with / before calling substring(1).
String servletPath = servletPathNode.asText();
if (servletPath != null && !servletPath.isEmpty() && servletPath.startsWith("/")) {
RESTCONF_CONFIG_ROOT_ELEMENT_NAME = servletPath.substring(1);
}| RESTCONF_CONFIG_ROOT_ELEMENT_NAME = configNode.get("restconf").get( | ||
| "restconfServletContextPath").asText().substring(1); |
There was a problem hiding this comment.
Similar to the previous occurrence, this code directly accesses nested JSON nodes without null checks, and assumes restconfServletContextPath starts with /. Add null checks and a check for / before calling substring(1) to prevent potential exceptions.
else {
JsonNode restconfNode = configNode.get("restconf");
if (restconfNode != null) {
JsonNode servletPathNode = restconfNode.get("restconfServletContextPath");
if (servletPathNode != null) {
String servletPath = servletPathNode.asText();
if (servletPath != null && !servletPath.isEmpty() && servletPath.startsWith("/")) {
RESTCONF_CONFIG_ROOT_ELEMENT_NAME = servletPath.substring(1);
}
}
}
}
d618a1d to
0b95350
Compare
|
/gemini review |
| public static final int MAXIMUM_FRAGMENT_LENGTH = 0; | ||
| public static final int IDLE_TIMEOUT = 30000; | ||
| public static final int HEARTBEAT_INTERVAL = 10000; | ||
| private static String RESTCONF_CONFIG_ENDPOINT_ELEMENT_NAME = "restconf"; |
There was a problem hiding this comment.
Introducing a mutable static field in a utility class can lead to thread-safety issues and unpredictable behavior in concurrent environments. Consider alternatives like passing the configuration value explicitly or using a ThreadLocal to store the value.
There was a problem hiding this comment.
We dont run Lighty.io in multiple instances and Lighty.io runs this code allways once during startup
| private static void hasRestconfServletContextPath(final JsonNode configNode){ | ||
| if (!configNode.get(RESTCONF_CONFIG_ROOT_ELEMENT_NAME).has("restconfServletContextPath")) { | ||
| LOG.warn("Json config does not contain restconfServletContextPath element. Using default."); | ||
| } else { | ||
| RESTCONF_CONFIG_ENDPOINT_ELEMENT_NAME = configNode.get("restconf").get( | ||
| "restconfServletContextPath").asText().substring(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
The call to .substring(1) is unsafe as it assumes the path string always starts with / and has a length greater than 1. Validate the configured restconfServletContextPath to prevent StringIndexOutOfBoundsException at runtime. Also, configNode.get("restconf") should use the RESTCONF_CONFIG_ROOT_ELEMENT_NAME constant.
private static void hasRestconfServletContextPath(final JsonNode configNode){
final JsonNode restconfNode = configNode.get(RESTCONF_CONFIG_ROOT_ELEMENT_NAME);
if (!restconfNode.has("restconfServletContextPath")) {
LOG.warn("Json config does not contain restconfServletContextPath element. Using default.");
} else {
final String path = restconfNode.get("restconfServletContextPath").asText();
if (path != null && path.startsWith("/") && path.length() > 1) {
RESTCONF_CONFIG_ENDPOINT_ELEMENT_NAME = path.substring(1);
} else if ("/".equals(path)) {
RESTCONF_CONFIG_ENDPOINT_ELEMENT_NAME = "";
} else {
LOG.warn("Value of restconfServletContextPath ('{}') is invalid. Using default.", path);
}
}
}There was a problem hiding this comment.
We allways set this field with /
| } else { | ||
| hasRestconfServletContextPath(configNode); | ||
| } |
There was a problem hiding this comment.
The else block calling hasRestconfServletContextPath is duplicated in the overloaded getRestConfConfiguration method. Extract this common logic into a private helper method to adhere to DRY principles.
There was a problem hiding this comment.
Most of getRestConfConfiguration() method is duplicated with the other one, this would require us to completly rework this class.
617be99 to
f13e759
Compare
JaxRs rework broke configurability of restconf base path, this patch resolves this issue by providing the path directly to JaxRsEndpoint. JIRA: LIGHTY-375 Signed-off-by: tobias.pobocik <tobias.pobocik@pantheon.tech>
2 participants
RESTCONF_CONFIG_ROOT_ELEMENT_NAME was hard-coded to be /restconf and the logic to apply different endpoint is no longer present after JaxRS rework.
JIRA: LIGHTY-375