Skip to content
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

Merged
merged 1 commit into from Sep 4, 2023

Conversation

Bhavya031
Copy link
Contributor

@Bhavya031 Bhavya031 commented Mar 17, 2023

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.

  • DONE

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

  • DONE

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:

  • No changelog needed

@github-actions
Copy link
Contributor

github-actions bot commented Mar 17, 2023

Suggested tests to cover this Pull Request
  • minssh_salt_install_package
  • min_deblike_monitoring
  • srv_power_management
  • min_salt_mgrcompat_state
  • srv_sync_channels
  • proxy_cobbler_pxeboot
  • allcli_software_channels_dependencies
  • srv_content_lifecycle
  • minkvm_guests
  • srv_task_status_engine
  • srv_patches_page
  • srv_logfile
  • srv_rename_hostname
  • srv_docker_cve_audit
  • srv_enable_sync_products
  • srv_create_repository
  • srv_cobbler_profile
  • min_empty_system_profiles
  • min_ansible_control_node
  • srv_reportdb
  • min_retracted_patches
  • srv_add_rocky8_repositories
  • srv_organization_credentials
  • allcli_reboot
  • min_bootstrap_ssh_key
  • srv_manage_activationkey
  • min_deblike_salt_install_package
  • buildhost_docker_build_image
  • srv_custom_system_info
  • minssh_bootstrap_api
  • srv_advanced_search
  • srv_cobbler_distro
  • srv_scc_user_credentials
  • min_rhlike_ssh
  • allcli_update_activationkeys
  • srv_datepicker
  • min_salt_openscap_audit
  • srv_change_password
  • srv_user_preferences
  • srv_cobbler_sync
  • min_salt_install_with_staging
  • allcli_config_channel
  • min_salt_software_states
  • srv_channels_add
  • min_bootstrap_api
  • min_bootstrap_reactivation
  • min_rhlike_monitoring
  • srv_distro_cobbler
  • min_rhlike_salt_install_package_and_patch
  • min_config_state_channel_api
  • buildhost_osimage_build_image
  • srv_group_union_intersection
  • allcli_action_chain
  • min_salt_pkgset_beacon
  • buildhost_bootstrap
  • proxy_branch_network
  • srv_virtual_host_manager
  • min_cve_id_new_syntax
  • allcli_sanity
  • min_deblike_ssh
  • srv_check_channels_page
  • srv_dist_channel_mapping
  • min_config_state_channel
  • srv_change_task_schedule
  • min_salt_minions_page
  • srv_delete_channel_with_tool
  • srv_notifications
  • min_deblike_remote_command
  • sle_ssh_minion
  • srv_wait_for_reposync
  • srv_salt
  • min_custom_pkg_download_endpoint
  • allcli_software_channels
  • srv_maintenance_windows
  • sle_minion
  • srv_cobbler_buildiso
  • minssh_move_from_and_to_proxy
  • srv_users
  • srv_user_api
  • srv_push_package
  • min_docker_api
  • srv_check_sync_source_packages
  • srv_sync_products
  • allcli_system_group
  • srv_handle_config_channels_with_ISS_v2
  • srv_delete_channel_from_ui
  • srv_restart
  • srv_power_management_redfish
  • srv_user_configuration_salt_states
  • min_bootstrap_negative
  • min_activationkey
  • srv_clone_channel_npn
  • min_rhlike_salt
  • min_salt_formulas_advanced
  • srv_handle_software_channels_with_ISS_v2
  • srv_manage_channels_page
  • srv_mainpage
  • min_salt_migration
  • min_recurring_action
  • proxy_as_pod_basic_tests
  • min_salt_minion_details
  • min_salt_lock_packages
  • min_deblike_openscap_audit
  • srv_menu_filter
  • srv_check_reposync
  • min_check_patches_install
  • min_virthost
  • srv_power_management_api
  • min_move_from_and_to_proxy
  • min_project_lotus
  • min_config_state_channel_subscriptions
  • srv_payg_ssh_connection
  • min_timezone
  • min_salt_user_states
  • srv_activationkey_api
  • minssh_ansible_control_node
  • min_deblike_salt
  • srv_docker
  • buildhost_docker_auth_registry
  • srv_monitoring
  • min_deblike_salt_install_with_staging
  • min_cve_audit
  • min_rhlike_remote_command
  • srv_channel_api
  • proxy_retail_pxeboot_and_mass_import
  • min_bootstrap_script
  • min_rhlike_openscap_audit
  • srv_docker_advanced_content_management
  • srv_salt_download_endpoint
  • proxy_register_as_minion_with_script
  • min_salt_install_package
  • srv_menu
  • min_monitoring
  • min_change_software_channel
  • min_action_chain
  • srv_osimage
  • srv_first_settings
  • minssh_action_chain
  • srv_create_activationkey
  • min_salt_formulas
  • allcli_overview_systems_details
  • srv_disable_local_repos_off
  • min_ssh_tunnel

@Etheryte Etheryte requested review from cbosdo and removed request for a team March 17, 2023 18:42
Copy link
Contributor

@cbosdo cbosdo left a 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);
Copy link
Contributor

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.

Copy link
Contributor

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);
Copy link
Contributor

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

Copy link
Contributor

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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same on those two

Copy link
Contributor

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) {
Copy link
Contributor

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

@@ -113,7 +113,7 @@ public ActionForward execute(ActionMapping mapping,

/** {@inheritDoc} */
@Override
public DataResult<AuditReviewDto> getResult(RequestContext context) {
public DataResult<AuditReviewDto> getResult(RequestContext context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation issue

@Bhavya031
Copy link
Contributor Author

@cbosdo I have run Java checklist it before making the pull request. I will attach a photo here as proof
Screenshot from 2023-03-21 13-23-02
. Also, I would like to assure you that all the changes I have made were done carefully and thoughtfully by me. In the next commit, I will address all of the changes you have suggested.

@cbosdo
Copy link
Contributor

cbosdo commented Mar 21, 2023

@cbosdo I have run Java checklist it before making the pull request. I will attach a photo here as proof Screenshot from 2023-03-21 13-23-02 . Also, I would like to assure you that all the changes I have made were done carefully and thoughtfully by me. In the next commit, I will address all of the changes you have suggested.

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.

@Bhavya031
Copy link
Contributor Author

@cbosdo I have run Java checklist it before making the pull request. I will attach a photo here as proof Screenshot from 2023-03-21 13-23-02 . Also, I would like to assure you that all the changes I have made were done carefully and thoughtfully by me. In the next commit, I will address all of the changes you have suggested.

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?

@cbosdo
Copy link
Contributor

cbosdo commented Mar 21, 2023

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 Map<String, Object> and all the places where this is used. The idea is to change one type and propagate.

@Bhavya031
Copy link
Contributor Author

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 Map<String, Object> and all the places where this is used. The idea is to change one type and propagate.

Great! I will start working as soon as possible. Also, what are we going to do about this pull requst?

@cbosdo
Copy link
Contributor

cbosdo commented Mar 21, 2023

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

@Bhavya031
Copy link
Contributor Author

Bhavya031 commented Mar 22, 2023

Hey, @cbosdo. I am converting this pull request specifically for Map<String, Object>. However, there are so many maps without parameters. Should I include all those maps in this pull request or should I limit it to a specific number of files or changes?

@cbosdo
Copy link
Contributor

cbosdo commented Mar 23, 2023

Hey, @cbosdo. I am converting this pull request specifically for Map<String, Object>. However, there are so many maps without parameters. Should I include all those maps in this pull request or should I limit it to a specific number of files or changes?

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 git rebase -i origin/master and removing the lines for the commits you don't want in the PR.

@Bhavya031
Copy link
Contributor Author

Bhavya031 commented Mar 25, 2023

Hey, @cbosdo. I am converting this pull request specifically for Map<String, Object>. However, there are so many maps without parameters. Should I include all those maps in this pull request or should I limit it to a specific number of files or changes?

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 git rebase -i origin/master and removing the lines for the commits you don't want in the PR.

@cbosdo I finished adding parameters to all the Map that use <String, Object>. However, some Map need different parameters for their unique use. Let me know if you find any issues with my pull request, and I'll fix it as soon as possible. Also, are you interested in being my mentor for a Gsoc project that listed in this GitHub issue: openSUSE/mentoring#197? The listed mentors aren't responding in the Gitter chat. Could you review my GSoC proposal as well?

Copy link
Contributor

@cbosdo cbosdo left a 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?

Comment on lines 104 to 105
for (Object itemObject : results) {
Map item = (Map) itemObject;
Map<String, Object> item = (Map<String, Object>) itemObject;
Copy link
Contributor

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>>

Copy link
Contributor

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This too

Copy link
Contributor

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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;
Copy link
Contributor

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too

@Bhavya031
Copy link
Contributor Author

I apologize for sitting so long on your PR. I just added a few comments / questions. Would you have time to look into them?

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

ivy:
[ivy-retrieve] :: Apache Ivy 2.5.1 - 20221101102211 :: https://ant.apache.org/ivy/ ::
[ivy-retrieve] :: loading settings :: file = /Users/bhavya/Developer/Java/opensourse/uyuni/java/buildconf/ivy/ivyconf.xml
[ivy-retrieve] :: resolving dependencies :: suse#manager;working@192.168.1.6
[ivy-retrieve] 	confs: [default]
[ivy-retrieve] :: resolution report :: resolve 508ms :: artifacts dl 37ms
	---------------------------------------------------------------------
	|                  |            modules            ||   artifacts   |
	|       conf       | number| search|dwnlded|evicted|| number|dwnlded|
	---------------------------------------------------------------------
	|      default     |  134  |   0   |   0   |   0   ||  155  |   0   |
	---------------------------------------------------------------------
[ivy-retrieve]
[ivy-retrieve] :: problems summary ::
[ivy-retrieve] :::: WARNINGS
[ivy-retrieve] 		module not found: suse#snakeyaml;1.31
[ivy-retrieve] 	==== suse: tried
[ivy-retrieve] 	  -- artifact suse#snakeyaml;1.31!snakeyaml.jar:
[ivy-retrieve] 	  /Users/bhavya/Developer/Java/opensourse/uyuni/java/buildconf/ivy/repository/suse/snakeyaml/1.31/snakeyaml-1.31.jar
[ivy-retrieve] 		::::::::::::::::::::::::::::::::::::::::::::::
[ivy-retrieve] 		::          UNRESOLVED DEPENDENCIES         ::
[ivy-retrieve] 		::::::::::::::::::::::::::::::::::::::::::::::
[ivy-retrieve] 		:: suse#snakeyaml;1.31: not found
[ivy-retrieve] 		::::::::::::::::::::::::::::::::::::::::::::::
[ivy-retrieve]
[ivy-retrieve] :: USE VERBOSE OR DEBUG MESSAGE LEVEL FOR MORE DETAILS

BUILD FAILED
/Users/bhavya/Developer/Java/opensourse/uyuni/java/manager-build.xml:72: impossible to resolve dependencies:
	resolve failed - see output for details

Total time: 6 seconds

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?

@cbosdo
Copy link
Contributor

cbosdo commented Jul 25, 2023

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

No we have moved to 1.33. You should rebase on master to get the ivy configuration change.

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?

Just get it into this one

@Bhavya031 Bhavya031 requested review from a team and cbbayburt as code owners July 25, 2023 09:12
@Bhavya031 Bhavya031 removed the request for review from a team July 25, 2023 09:12
@nodeg
Copy link
Member

nodeg commented Jul 26, 2023

Removing QE here, since no test suite code is touched.

@deneb-alpha
Copy link
Contributor

removing release-engineering since there's nothing to review for us for now. feel free to re-add us later if/when needed

@deneb-alpha deneb-alpha removed the request for review from a team July 26, 2023 13:52
@Bhavya031
Copy link
Contributor Author

@cbosdo, I addressed your comments. If everything is alright, then we are good to go.

Copy link
Contributor

@cbosdo cbosdo left a 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);
Copy link
Contributor

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);
Copy link
Contributor

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());
Copy link
Contributor

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) {
Copy link
Contributor

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);
Copy link
Contributor

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

Comment on lines 644 to 646
if (log.isDebugEnabled()) {
log.debug("Final path before returning getStreamForPath(): {}", diskPath);
}
Copy link
Contributor

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 = "";
Copy link
Contributor

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;
Copy link
Contributor

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;
Copy link
Contributor

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;
Copy link
Contributor

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

@Bhavya031
Copy link
Contributor Author

I left a few comments that would need to be addressed before merging.

I addressed all of your comments. Can you give me any updates?

@meaksh meaksh removed their request for review August 30, 2023 12:48
@Bhavya031
Copy link
Contributor Author

@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.
@cbosdo
Copy link
Contributor

cbosdo commented Sep 4, 2023

@cbosdo It's been a month since I addressed your comments. Can you give me a review or any feedback?

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

@cbosdo cbosdo merged commit ce48554 into uyuni-project:master Sep 4, 2023
15 checks passed
@cbosdo
Copy link
Contributor

cbosdo commented Sep 4, 2023

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 😉

@Bhavya031
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix unchecked types warnings.
5 participants