-
Notifications
You must be signed in to change notification settings - Fork 0
Refactor JobGetInfos to streamline route handling; add support for se… #60
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
Conversation
…ssion push and improve query parameter processing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the JobGetInfos class to streamline route handling by introducing a fluent API pattern with separate methods for body, params, and query parameters. It also adds support for session push functionality through a new route and improves query parameter processing.
- Simplified
getRoutes()method overloads from ~15 methods to 2, replacing them with a fluent API using chainedbody(),params(), andquery()methods - Added
SESSION_PUSHroute for authentication flow and corresponding test methods - Removed
ExampleUsages.javafile containing example code
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/main/java/fr/sandro642/github/jobs/JobGetInfos.java | Refactored to use fluent API with instance variable fullRoute and separate methods for body, params, and query; removed multiple overloaded getRoutes() methods |
| src/test/java/fr/sandro642/github/test/MainTest.java | Added SESSION_PUSH route, new test methods (testProd, pushsession), and updated existing test calls to use new API pattern |
| src/main/java/fr/sandro642/github/example/ExampleUsages.java | Deleted example usage file |
Comments suppressed due to low confidence (3)
src/test/java/fr/sandro642/github/test/MainTest.java:199
- Method name
pushsession()does not follow Java naming conventions for multi-word identifiers. It should be in camelCase (e.g.,pushSession()ortestPushSession()to match the test naming pattern).
System.out.println("Data: " + apiFactory.getData("data"));
src/test/java/fr/sandro642/github/test/MainTest.java:163
- Variable name
code_sessiondoes not follow Java naming conventions. It should be in camelCase (e.g.,codeSessionorsessionCode) instead of snake_case.
.execute();
src/test/java/fr/sandro642/github/test/MainTest.java:170
- The test
testProd()directly calls other test methods (testSpecData()andpushsession()), creating test dependencies. This violates test independence principles - tests should be atomic and executable in any order. Consider using a test suite, @beforeeach setup, or refactoring shared logic into helper methods instead.
ApiFactory response = factoryCompletableFuture.get(5, TimeUnit.SECONDS);
System.out.println("Response: " + response.display());
System.out.println("Status Code: " + response.getStatusCode());
} catch (Exception e) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public JobGetInfos params(Map<?, ?> params) { | ||
| if (params == null || params.isEmpty()) return this; | ||
|
|
||
| if (body != null) { | ||
| connectLib.StoreAndRetrieve().store.put("currentBody", body); | ||
| } | ||
| for (Map.Entry<?, ?> entry : params.entrySet()) { | ||
| String paramKey = "{" + entry.getKey() + "}"; | ||
| String paramValue = entry.getValue() == null ? "" : entry.getValue().toString(); | ||
| this.fullRoute = this.fullRoute.replace(paramKey, paramValue); | ||
| } | ||
|
|
||
| connectLib.StoreAndRetrieve().store.put("currentParams", params); | ||
| connectLib.StoreAndRetrieve().store.put("currentRoute", this.fullRoute); | ||
| return this; | ||
| } |
Copilot
AI
Nov 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing documentation for the newly added public method params(). Add JavaDoc to describe its purpose, parameters, and return value to maintain consistency with the existing codebase documentation standards.
| queryString.append("&"); | ||
| } | ||
| Object value = query.get(paramName); | ||
| queryString.append(paramName).append("=").append(value == null ? "" : value.toString()); |
Copilot
AI
Nov 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Query parameter values are not URL-encoded before being appended to the query string. Special characters in parameter values (e.g., spaces, &, =) will cause malformed URLs or incorrect query strings. Use URLEncoder.encode(value.toString(), StandardCharsets.UTF_8) to properly encode the values.
| public JobGetInfos params(Map<?, ?> params) { | ||
| if (params == null || params.isEmpty()) return this; | ||
|
|
||
| if (body != null) { | ||
| connectLib.StoreAndRetrieve().store.put("currentBody", body); | ||
| } | ||
| for (Map.Entry<?, ?> entry : params.entrySet()) { | ||
| String paramKey = "{" + entry.getKey() + "}"; | ||
| String paramValue = entry.getValue() == null ? "" : entry.getValue().toString(); | ||
| this.fullRoute = this.fullRoute.replace(paramKey, paramValue); | ||
| } | ||
|
|
||
| connectLib.StoreAndRetrieve().store.put("currentParams", params); | ||
| connectLib.StoreAndRetrieve().store.put("currentRoute", this.fullRoute); | ||
| return this; | ||
| } |
Copilot
AI
Nov 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The params() method modifies this.fullRoute but will fail if getRoutes() hasn't been called first, as fullRoute is initialized to empty string. This creates a fragile method call order dependency. Consider validating that fullRoute is not empty or documenting the required call order.
| */ | ||
| private URLProvider urlBranch; | ||
|
|
||
| private String fullRoute = ""; |
Copilot
AI
Nov 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instance variable fullRoute introduces thread safety issues. Multiple threads could concurrently call getRoutes(), params(), or query() on the same JobGetInfos instance, leading to race conditions where threads overwrite each other's route values. Consider making fullRoute a local variable passed between methods or using ThreadLocal storage.
| private String fullRoute = ""; | |
| // Removed fullRoute instance variable for thread safety. |
| public JobGetInfos query(Map<?, ?> query) { | ||
| if (query == null || query.isEmpty()) return this; | ||
|
|
||
| if (params != null) { | ||
| connectLib.StoreAndRetrieve().store.put("currentParams", params); | ||
| Pattern pattern = Pattern.compile("\\$([^$]+)\\$"); | ||
| Matcher matcher = pattern.matcher(this.fullRoute); | ||
|
|
||
| StringBuilder queryString = new StringBuilder("?"); | ||
| boolean firstParam = true; | ||
|
|
||
| while (matcher.find()) { | ||
| String paramName = matcher.group(1); | ||
| if (query.containsKey(paramName)) { | ||
| if (!firstParam) { | ||
| queryString.append("&"); | ||
| } | ||
| Object value = query.get(paramName); | ||
| queryString.append(paramName).append("=").append(value == null ? "" : value.toString()); | ||
| firstParam = false; | ||
| } | ||
| } | ||
|
|
||
| connectLib.Logger().INFO(connectLib.LangManager().getMessage(CategoriesType.JOBS_PACKAGE, "getroutes.maderoute", "route", fullRoute)); | ||
| } catch (Exception e) { | ||
| connectLib.Logger().ERROR(connectLib.LangManager().getMessage(CategoriesType.JOBS_PACKAGE, "getroutes.error", "exception", e.getMessage())); | ||
| if (queryString.length() > 1) { | ||
| String cleanRoute = this.fullRoute.replaceAll("\\$[^$]+\\$", "").replaceAll("&+", ""); | ||
| this.fullRoute = cleanRoute + queryString.toString(); | ||
| } | ||
|
|
||
| connectLib.StoreAndRetrieve().store.put("currentRoute", this.fullRoute); | ||
| connectLib.StoreAndRetrieve().store.put("currentQuery", query); | ||
| return this; | ||
| } |
Copilot
AI
Nov 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing documentation for the newly added public method query(). Add JavaDoc to describe its purpose, parameters, and return value to maintain consistency with the existing codebase documentation standards.
|
|
||
| System.out.println(apiFactory.getSpecData("data", "accessToken")); | ||
|
|
||
| this.code_session = (String) apiFactory.getSpecData("data", "sessionCode"); |
Copilot
AI
Nov 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential NullPointerException: this.code_session could be null if testSpecData() hasn't been executed first or if the API response doesn't contain the expected data. Add null checking before using this variable, especially since test execution order isn't guaranteed.
| public JobGetInfos query(Map<?, ?> query) { | ||
| if (query == null || query.isEmpty()) return this; | ||
|
|
||
| if (params != null) { | ||
| connectLib.StoreAndRetrieve().store.put("currentParams", params); | ||
| Pattern pattern = Pattern.compile("\\$([^$]+)\\$"); | ||
| Matcher matcher = pattern.matcher(this.fullRoute); | ||
|
|
||
| StringBuilder queryString = new StringBuilder("?"); | ||
| boolean firstParam = true; | ||
|
|
||
| while (matcher.find()) { | ||
| String paramName = matcher.group(1); | ||
| if (query.containsKey(paramName)) { | ||
| if (!firstParam) { | ||
| queryString.append("&"); | ||
| } | ||
| Object value = query.get(paramName); | ||
| queryString.append(paramName).append("=").append(value == null ? "" : value.toString()); | ||
| firstParam = false; | ||
| } | ||
| } | ||
|
|
||
| connectLib.Logger().INFO(connectLib.LangManager().getMessage(CategoriesType.JOBS_PACKAGE, "getroutes.maderoute", "route", fullRoute)); | ||
| } catch (Exception e) { | ||
| connectLib.Logger().ERROR(connectLib.LangManager().getMessage(CategoriesType.JOBS_PACKAGE, "getroutes.error", "exception", e.getMessage())); | ||
| if (queryString.length() > 1) { | ||
| String cleanRoute = this.fullRoute.replaceAll("\\$[^$]+\\$", "").replaceAll("&+", ""); | ||
| this.fullRoute = cleanRoute + queryString.toString(); | ||
| } | ||
|
|
||
| connectLib.StoreAndRetrieve().store.put("currentRoute", this.fullRoute); | ||
| connectLib.StoreAndRetrieve().store.put("currentQuery", query); | ||
| return this; | ||
| } |
Copilot
AI
Nov 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query() method modifies this.fullRoute but will fail if getRoutes() hasn't been called first, as fullRoute is initialized to empty string. This creates a fragile method call order dependency. Consider validating that fullRoute is not empty or documenting the required call order.
|
|
||
| if (versionType != null && versionType.getVersion() != null) { | ||
| String cleanRoute = route.startsWith("/") ? route.substring(1) : route; | ||
| fullRoute = "/" + versionType.getVersion() + "/" + cleanRoute; |
Copilot
AI
Nov 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Inconsistent use of this keyword: line 87 assigns to fullRoute without this, while line 89 uses this.fullRoute. For better code consistency and readability, use this.fullRoute in both places.
| fullRoute = "/" + versionType.getVersion() + "/" + cleanRoute; | |
| this.fullRoute = "/" + versionType.getVersion() + "/" + cleanRoute; |
| public JobGetInfos body(Map<?, ?> body) { | ||
| connectLib.StoreAndRetrieve().store.put("currentBody", body); | ||
| return this; | ||
| } |
Copilot
AI
Nov 15, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing documentation for the newly added public method body(). Add JavaDoc to describe its purpose, parameters, and return value to maintain consistency with the existing codebase documentation standards.
…ssion push and improve query parameter processing