-
Notifications
You must be signed in to change notification settings - Fork 180
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
Fix unchecked types warning around PackageMetadata #6750
Conversation
Suggested tests to cover this Pull Request
|
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.
Could you please handle one kind of generic types fixing in a single pull request? For instance you define the elaborator params type to Map<String, Object>
which is good, but I would see only this and all its uses addressed in one PR to make it easier to review.
When changing those types, make sure to read the code using them to figure out if the types you inferred are correct... and keep your fingers away from the datasource
package: it can bite pretty badly.
mode.elaborate(this, elaboratorParams); | ||
mode.elaborate(new ArrayList(), elaboratorParams); |
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.
Replacing this
by new ArrayList()
is dangerous. The elaborate()
function is called on a data set to run more queries on it. Better not touch that code unless you really know what it does: mode queries are complex beasts implementing something like Hibernate.
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.
Please, remove this change even if it leaves a sonarlint warning: the implications of that change are not clear enough and make it too risky.
@@ -145,7 +145,7 @@ public DataResult<T> slice(int fromIndex, int toIndex) { | |||
public void elaborate(Map<String, Object> values) { | |||
elabParams = values; | |||
if (mode != null) { | |||
mode.elaborate(this, values); | |||
mode.elaborate(new ArrayList<>(this), values); |
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.
Same here... likely to heavily change the behavior of the function
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.
Same for this change: please remove it even if it leaves a sonarlint error: too risky
} | ||
else { | ||
mode.elaborate(this, getElaborationParams()); | ||
mode.elaborate(new ArrayList<>(this), getElaborationParams()); |
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.
Same on those two
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.
Same for those two, please remove them as they are too risky as well.
@@ -134,10 +134,10 @@ public <T> DataResult<T> execute(Map<String, ?> parameters, List<?> inClause) { | |||
* results. | |||
* @param parameters named query parameters for elaborators. | |||
*/ | |||
public void elaborate(List resultList, Map<String, ?> parameters) { | |||
public void elaborate(List<Object> resultList, Map<String, ?> parameters) { |
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.
This one is rather likely to be parametrized
java/code/src/com/redhat/rhn/common/db/datasource/ModeElaborator.java
Outdated
Show resolved
Hide resolved
@@ -113,7 +113,7 @@ public ActionForward execute(ActionMapping mapping, | |||
|
|||
/** {@inheritDoc} */ | |||
@Override | |||
public DataResult<AuditReviewDto> getResult(RequestContext context) { | |||
public DataResult<AuditReviewDto> getResult(RequestContext context) { |
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.
Indentation issue
java/code/src/com/redhat/rhn/frontend/action/audit/scap/XccdfSearchAction.java
Show resolved
Hide resolved
java/code/src/com/redhat/rhn/frontend/action/audit/scap/XccdfSearchHelper.java
Outdated
Show resolved
Hide resolved
java/code/src/com/redhat/rhn/frontend/action/audit/scap/XccdfSearchHelper.java
Outdated
Show resolved
Hide resolved
@cbosdo I have run |
That it compiles is a good step, but this does not mean it will be fine at run time. To get such changes under control I would rather have multiple smaller PRs: it makes them easier to review and merge what is already good. |
How about I categorize all things and create a PR for each category, such as lists, maps, and so on? |
Split by kind of change. For instance you could have one PR with the elaborator parameters typed as |
Great! I will start working as soon as possible. Also, what are we going to do about this pull requst? |
You can keep it and reduce the number of changes in it. Don't be afraid to force push to your branch: this will help you refactor your commits. You can simply edit the title after to match what the PR really changes |
Hey, @cbosdo. I am converting this pull request specifically for |
Just keep those for the elaborator parameters in this pull request. The other maps are most likely unrelated. Also remove the first commits from this pull request. This can easily be done using |
@cbosdo I finished adding parameters to all the |
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.
I apologize for sitting so long on your PR. I just added a few comments / questions. Would you have time to look into them?
java/code/src/com/redhat/rhn/frontend/action/channel/PackageSearchHelper.java
Outdated
Show resolved
Hide resolved
for (Object itemObject : results) { | ||
Map item = (Map) itemObject; | ||
Map<String, Object> item = (Map<String, Object>) itemObject; |
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.
This could be simplified if we can the results to List<Map<String, Object>>
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.
Please change to
- for (Object itemObject : results) {
- Map item = (Map) itemObject;
+ for (Map<String, Object> item : results) {
@@ -147,7 +147,7 @@ public static List<PackageOverview> performSearch(Long sessionId, String searchS | |||
// Iterate through in the order that the search server returned, add packages | |||
// to the return list in the order they appear in the search results. | |||
for (Object resultObject : results) { | |||
Map result = (Map) resultObject; | |||
Map<String, Object> result = (Map<String, Object>) resultObject; |
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.
This too
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.
Please change to
- for (Object resultObject : results) {
- Map result = (Map) resultObject;
+ for (Map<String, Object> result : results) {
@@ -268,7 +268,7 @@ private void removeObjectFromSet(List<String> keys, Object obj) { | |||
} | |||
} | |||
else if (obj instanceof Map) { | |||
Map next = (Map) obj; | |||
Map<String, Object> next = (Map) obj; |
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.
Map<String, Object> next = (Map) obj; | |
Map<String, Object> next = (Map<String, Object>) obj; |
@@ -310,7 +310,7 @@ private void syncObjectToSet(Set set, Object obj) { | |||
} | |||
} | |||
else if (obj instanceof Map) { | |||
Map next = (Map) obj; | |||
Map<String, Object> next = (Map) obj; |
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.
same here
@@ -363,7 +363,7 @@ private void addObjectToSet(Set set, Object obj) { | |||
} | |||
} | |||
else if (obj instanceof Map) { | |||
Map next = (Map) obj; | |||
Map<String, Object> next = (Map) obj; |
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.
here too
Sure, I am looking into that as soon as possible. I am having trouble setting up the development environment on my Mac. Regarding that, are we still using snakeyaml-1.31.jar? I am getting this
error while building the dependency. Also, there is one line update in the documentation. Should I add that in this pull request, or should I open another one? |
No we have moved to 1.33. You should rebase on
Just get it into this one |
Removing QE here, since no test suite code is touched. |
removing release-engineering since there's nothing to review for us for now. feel free to re-add us later if/when needed |
@cbosdo, I addressed your comments. If everything is alright, then we are good to go. |
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.
I left a few comments that would need to be addressed before merging.
mode.elaborate(this, elaboratorParams); | ||
mode.elaborate(new ArrayList(), elaboratorParams); |
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.
Please, remove this change even if it leaves a sonarlint warning: the implications of that change are not clear enough and make it too risky.
@@ -145,7 +145,7 @@ public DataResult<T> slice(int fromIndex, int toIndex) { | |||
public void elaborate(Map<String, Object> values) { | |||
elabParams = values; | |||
if (mode != null) { | |||
mode.elaborate(this, values); | |||
mode.elaborate(new ArrayList<>(this), values); |
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.
Same for this change: please remove it even if it leaves a sonarlint error: too risky
} | ||
else { | ||
mode.elaborate(this, getElaborationParams()); | ||
mode.elaborate(new ArrayList<>(this), getElaborationParams()); |
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.
Same for those two, please remove them as they are too risky as well.
@@ -113,7 +113,7 @@ protected String getExportColumns(HttpServletRequest request, HttpSession sessio | |||
* @param session HTTP session | |||
* @return page data | |||
*/ | |||
protected List<BaseDto> getPageData(HttpServletRequest request, HttpSession session) { | |||
protected List<Object> getPageData(HttpServletRequest request, HttpSession session) { |
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.
Why changing from BaseDto
to Object
here? If we return anything different than a BaseDto
we're likely to have troubles somewhere else.
@@ -179,7 +179,7 @@ protected StreamInfo getStreamInfo(ActionMapping mapping, ActionForm form, | |||
} | |||
|
|||
String exportColumns = getExportColumns(request, session); | |||
List<BaseDto> pageData = getPageData(request, session); | |||
List<Object> pageData = getPageData(request, session); |
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.
Same for this one: you should not need to change from BaseDto
to Object
if (log.isDebugEnabled()) { | ||
log.debug("Final path before returning getStreamForPath(): {}", diskPath); | ||
} |
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.
Please remove this change: it's useless to have that log.debug()
in an if
branch and is a duplicate with the line above
@@ -362,7 +362,7 @@ private ActionForward handleUserDownload(HttpServletRequest request, String url, | |||
protected StreamInfo getStreamInfo(ActionMapping mapping, ActionForm form, | |||
HttpServletRequest request, HttpServletResponse response) throws Exception { | |||
|
|||
String path; | |||
String path = ""; |
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.
Rather than setting this variable to ""
remove it from here and declare it where it is used.
@@ -51,7 +51,7 @@ private List<String> createDependenciesStrings(DataResult dr) { | |||
|
|||
// Loop through all items in data result | |||
for (Object oIn : dr) { | |||
Map item = (Map) oIn; | |||
Map<String, Object> item = (Map) oIn; |
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.
Better fix the DataResult
type in the parameter as well.
@@ -236,7 +236,7 @@ private String getEmailBodyAffectedSystems(String host, List servers) { | |||
StringWriter writer = new StringWriter(); | |||
PrintWriter printWriter = new PrintWriter(writer, true); | |||
for (Object serverIn : servers) { | |||
Map row = (Map) serverIn; | |||
Map<String, Object> row = (Map<String, Object>) serverIn; |
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.
Better fix the type of the servers
parameter to simplify this even more
@@ -69,7 +69,7 @@ public void execute(JobExecutionContext ctx) throws JobExecutionException { | |||
return; | |||
} | |||
for (Object oIn : dr) { | |||
Map row = (Map) oIn; | |||
Map<String, Object> row = (Map<String, Object>) oIn; |
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.
Same here, fix, the dr
type and simplify here
I addressed all of your comments. Can you give me any updates? |
@cbosdo It's been a month since I addressed your comments. Can you give me a review or any feedback? |
In this commit, I am specifying generic types where they were not specified. I used the SonarCloud report as a reference to know where to specify.
I just fixed the remaining issues I saw and force pushed to your branch after a rebase. I'll wait for the tests to go green before merging |
The tests went green. Thanks a lot for your contribution and I apologize for taking so long to review it. Please file smaller PRs for easier review next time 😉 |
Thank you a lot. You are the best. This is my first contribution, and you were so helpful. |
What does this PR change?
add description
The recent commit addressed several unchecked type warnings in PackageMetadata by introducing generics. While determining the optimal scope of the commit was a challenge, it successfully eliminated warnings in several files.
GUI diff
No difference.
Documentation
No documentation needed: just spacifying genaric type in java metadata
DONE
Test coverage
No tests: no new code, just easy refactoring
DONE
Links
Fixes #612
Changelogs
Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository
If you don't need a changelog check, please mark this checkbox: