[ZEPPELIN-6309] Improve method by replacing JsonObject parameter by celinayk · Pull Request #5063 · apache/zeppelin

celinayk · 2025-08-31T13:29:54Z

What is this PR for?

Refactored the convertPermissionsFromUsersToOwners method in InterpreterSetting.java to improve code readability and maintainability by separating JSON parsing logic from business logic, addressing the TODO comment that identified this as "ugly code".

What type of PR is it?

Refactoring

Todos

- Refactor convertPermissionsFromUsersToOwners method to remove JsonObject parameter
- Extract JSON parsing logic into separate static helper method
- Update all callers to use new method signature

What is the Jira issue?

ZEPPELIN-6309

How should this be tested?

Screenshots (if appropriate)

Questions:

Does the license files need to update? No
Is there breaking changes for older versions? No
Does this needs documentation? No

Copilot

Pull Request Overview

This PR refactors the convertPermissionsFromUsersToOwners method in InterpreterSetting.java by removing the JsonObject parameter and separating JSON parsing logic from business logic, addressing a TODO comment about "ugly code".

Extracted JSON parsing into a new static helper method extractUsersFromJsonObject
Simplified the main method to accept a List<String> instead of JsonObject
Updated all callers to use the new method signature with proper JSON parsing

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
InterpreterSetting.java	Refactored main method and added new static helper method for JSON parsing
ConfigStorage.java	Updated caller to use new method signature with extracted users list
InterpreterInfoSaving.java	Updated caller to use new method signature with extracted users list

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-01T12:57:29Z

+ List<String> users = new ArrayList<>();
 if (jsonObject != null) {
 JsonObject option = jsonObject.getAsJsonObject("option");
 if (option != null) {
- JsonArray users = option.getAsJsonArray("users");
- if (users != null) {
- if (this.option.getOwners() == null) {
- this.option.owners = new LinkedList<>();
- }
- for (JsonElement user : users) {
- this.option.getOwners().add(user.getAsString());
+ JsonArray usersArray = option.getAsJsonArray("users");
+ if (usersArray != null) {
+ for (JsonElement userElement : usersArray) {
+ users.add(userElement.getAsString());
 }
 }
 }
 }
+ return users;


[nitpick] Consider using ArrayList constructor with initial capacity or LinkedList to match the pattern used in the convertPermissionsFromUsersToOwners method where LinkedList is used for owners.
Suggested change
List<String> users = newArrayList<>();
if (jsonObject != null) {
JsonObjectoption = jsonObject.getAsJsonObject("option");
if (option != null) {
JsonArrayusers = option.getAsJsonArray("users");
if (users != null) {
if (this.option.getOwners() == null) {
this.option.owners = newLinkedList<>();
}
for (JsonElementuser : users) {
this.option.getOwners().add(user.getAsString());
JsonArrayusersArray = option.getAsJsonArray("users");
if (usersArray != null) {
for (JsonElementuserElement : usersArray) {
users.add(userElement.getAsString());
}
}
}
}
returnusers;
List<String> users = null;
if (jsonObject != null) {
JsonObjectoption = jsonObject.getAsJsonObject("option");
if (option != null) {
JsonArrayusersArray = option.getAsJsonArray("users");
if (usersArray != null) {
users = newArrayList<>(usersArray.size());
for (JsonElementuserElement : usersArray) {
users.add(userElement.getAsString());
}
}
}
}
returnusers == null ? newArrayList<>() : users;

Good catch! Updated to use LinkedList for consistency with the rest of the class. This maintains consistency with the owners field initialization on line.

…figStorage.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

…nObject method

Reamer · 2025-09-03T10:05:37Z

- interpreterSetting.convertPermissionsFromUsersToOwners(
- jsonObject.getAsJsonObject("interpreterSettings")
- .getAsJsonObject(interpreterSetting.getId()));
+ JsonObject interpreterSettingJson = jsonObject.getAsJsonObject("interpreterSettings")


Are you sure this is what is meant by “ugly” here?
I think we should use GSON here to create objects directly.
GSON.fromJson(json, myFanceClass.class);

Are you sure this is what is meant by “ugly” here? I think we should use GSON here to create objects directly.
GSON.fromJson(json, myFanceClass.class);
Thank you for the feedback! You're absolutely right. I've refactored the code to use GSON.fromJson() directly
instead of JsonObject parameter.

…ct parameter

ParkGyeongTae

LGTM 👍

### What is this PR for? Refactored the `convertPermissionsFromUsersToOwners` method in InterpreterSetting.java to improve code readability and maintainability by separating JSON parsing logic from business logic, addressing the TODO comment that identified this as "ugly code". ### What type of PR is it? Refactoring ### Todos * [x] - Refactor convertPermissionsFromUsersToOwners method to remove JsonObject parameter * [x] - Extract JSON parsing logic into separate static helper method * [x] - Update all callers to use new method signature ### What is the Jira issue? [ZEPPELIN-6309](https://issues.apache.org/jira/browse/ZEPPELIN-6309) ### How should this be tested? ### Screenshots (if appropriate) ### Questions: * Does the license files need to update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Closes#5063 from celinayk/ZEPPELIN-6309. Signed-off-by: ParkGyeongTae <gyeongtae@apache.org> (cherry picked from commit 75a9caa) Signed-off-by: ParkGyeongTae <gyeongtae@apache.org>

ParkGyeongTae · 2025-09-15T04:05:55Z

Merged into master and branch-0.12

…ers.json ### What is this PR for? Tests in the `zeppelin-zengine` module were failing. The main reason was that the `interpreter.json` in test resources were invalid: the value object did not have an `id` field matching its corresponding key. As a result, the deserialized `InterpreterSetting` instance had an auto-genarated `id` field, causing the key and value.id to be inconsistent. Before #5063, invalid settings were simply skipped. However, after that change, they caused a NPE. This PR fixes the invalid JSON files and adds minor validation logic in the deserialization method so that such issues can be detected early by users. ### What type of PR is it? Bug Fix ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-6335 ### How should this be tested? * Check `core-modules` - `zeppelin-zengine` tests in CI ### Questions: * Does the license files need to update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Closes#5081 from tbonelee/fix-test-resource. Signed-off-by: ChanHo Lee <chanholee@apache.org>

…ers.json ### What is this PR for? Tests in the `zeppelin-zengine` module were failing. The main reason was that the `interpreter.json` in test resources were invalid: the value object did not have an `id` field matching its corresponding key. As a result, the deserialized `InterpreterSetting` instance had an auto-genarated `id` field, causing the key and value.id to be inconsistent. Before #5063, invalid settings were simply skipped. However, after that change, they caused a NPE. This PR fixes the invalid JSON files and adds minor validation logic in the deserialization method so that such issues can be detected early by users. ### What type of PR is it? Bug Fix ### What is the Jira issue? https://issues.apache.org/jira/browse/ZEPPELIN-6335 ### How should this be tested? * Check `core-modules` - `zeppelin-zengine` tests in CI ### Questions: * Does the license files need to update? No * Is there breaking changes for older versions? No * Does this needs documentation? No Closes#5081 from tbonelee/fix-test-resource. Signed-off-by: ChanHo Lee <chanholee@apache.org> (cherry picked from commit 80ff51c) Signed-off-by: ChanHo Lee <chanholee@apache.org>

[ZEPPELIN-6309] Improve method by replacing JsonObject parameter
aa9b6fe

jongyoul requested a review from Copilot September 1, 2025 12:56

CopilotAI reviewed Sep 1, 2025
View reviewed changes

celinaykand others added 2 commits September 2, 2025 18:27

Update zeppelin-zengine/src/main/java/org/apache/zeppelin/storage/Con…
92b371e
…figStorage.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

[ZEPPELIN-6309] Use LinkedList for consistency in extractUsersFromJso…
e41ed80
…nObject method

Reamer reviewed Sep 3, 2025
View reviewed changes

[ZEPPELIN-6309] Refactor JSON parsing to use GSON instead of JsonObje…
6a628e3
…ct parameter

ParkGyeongTae approved these changes Sep 7, 2025
View reviewed changes

ParkGyeongTae merged commit 75a9caa into apache:masterSep 15, 2025
14 of 18 checks passed

ParkGyeongTae self-assigned this Sep 15, 2025

tbonelee mentioned this pull request Sep 22, 2025
[ZEPPELIN-6335] Fix failing zengine tests caused by invalid interpreters.json #5081
Merged

Navigation Menu

Uh oh!

[ZEPPELIN-6309] Improve method by replacing JsonObject parameter - #5063

[ZEPPELIN-6309] Improve method by replacing JsonObject parameter#5063
ParkGyeongTae merged 4 commits into
apache:masterfrom
celinayk:ZEPPELIN-6309

celinayk commented Aug 31, 2025

Uh oh!

CopilotAI left a comment

Uh oh!

CopilotAISep 1, 2025

Uh oh!

celinaykSep 2, 2025

Uh oh!

Uh oh!

ReamerSep 3, 2025

Uh oh!

celinaykSep 4, 2025

Uh oh!

ParkGyeongTae left a comment

Uh oh!

Uh oh!

ParkGyeongTae commented Sep 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

-List<String> users = newArrayList<>();
-if (jsonObject != null) {
-JsonObjectoption = jsonObject.getAsJsonObject("option");
-if (option != null) {
-JsonArrayusers = option.getAsJsonArray("users");
-if (users != null) {
-if (this.option.getOwners() == null) {
-this.option.owners = newLinkedList<>();
- }
-for (JsonElementuser : users) {
-this.option.getOwners().add(user.getAsString());
-JsonArrayusersArray = option.getAsJsonArray("users");
-if (usersArray != null) {
-for (JsonElementuserElement : usersArray) {
-users.add(userElement.getAsString());
- }
- }
- }
- }
-returnusers;
+List<String> users = null;
+if (jsonObject != null) {
+JsonObjectoption = jsonObject.getAsJsonObject("option");
+if (option != null) {
+JsonArrayusersArray = option.getAsJsonArray("users");
+if (usersArray != null) {
+users = newArrayList<>(usersArray.size());
+for (JsonElementuserElement : usersArray) {
+users.add(userElement.getAsString());
+ }
+ }
+ }
+ }
+returnusers == null ? newArrayList<>() : users;

Uh oh!

Conversation

celinayk commented Aug 31, 2025

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

CopilotAISep 1, 2025

Choose a reason for hiding this comment

Uh oh!

celinaykSep 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ReamerSep 3, 2025

Choose a reason for hiding this comment

Uh oh!

celinaykSep 4, 2025

Choose a reason for hiding this comment

Uh oh!

ParkGyeongTae left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ParkGyeongTae commented Sep 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants