Skip to content

Prevent NullPointerException on GenericDaoBase#4268

Merged
yadvr merged 2 commits intoapache:masterfrom
CLDIN:null-at-generic-dao-base
Aug 17, 2020
Merged

Prevent NullPointerException on GenericDaoBase#4268
yadvr merged 2 commits intoapache:masterfrom
CLDIN:null-at-generic-dao-base

Conversation

@GabrielBrascher
Copy link
Member

Description

Recently I had a few DB connection/transaction issues on a test environment and one of the log messages was the following NullPointerException:

2020-08-12 22:48:33,250 WARN  [c.c.a.m.AgentManagerImpl] (AgentManager-Handler-1:null) (logid:) Caught: 
java.lang.NullPointerException
        at com.cloud.utils.db.GenericDaoBase.update(GenericDaoBase.java:853)
        at com.cloud.utils.db.GenericDaoBase.update(GenericDaoBase.java:809)
        at com.cloud.utils.db.GenericDaoBase.update(GenericDaoBase.java:1365)

Looking further we can see that the null pointer was caused due to a null Object calling equals method. A simple fix would be to use the static final not-null Object calling the Object.equals method.

This PR also extracted the duplicated code that could cause NullPointerException and added unit tests to ensure that no NullPointerException will be thrown.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

@GabrielBrascher GabrielBrascher added this to the 4.15.0.0 milestone Aug 15, 2020
@GabrielBrascher GabrielBrascher self-assigned this Aug 15, 2020
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.runners.MockitoJUnitRunner;
import org.mockito.junit.MockitoJUnitRunner;
Copy link
Member Author

Choose a reason for hiding this comment

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

Previous MockitoJUnitRunner got Deprecated; changed to a non-deprecated one.

public void prepareTests() throws SQLException {
Mockito.when(resultSet.getObject(1)).thenReturn(false);
Mockito.when(resultSet.getBoolean(1)).thenReturn(false);
Mockito.when(resultSet.getObject(1)).thenReturn((short) 1);
Copy link
Member Author

Choose a reason for hiding this comment

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

Despite all tests pass, the Process finished with exit code 255 due to unnecessary stubbings.
I fixed this issue by extracting all into a @Before method and tests are looking good.

The output of tests before this PR:

Process finished with exit code 255

org.mockito.exceptions.misusing.UnnecessaryStubbingException: 
Unnecessary stubbings detected in test class: GenericDaoBaseTest
Clean & maintainable test code requires zero unnecessary code.
Following stubbings are unnecessary (click to navigate to relevant line of code):
  1. -> at com.cloud.utils.db.GenericDaoBaseTest.getObjectPrimitiveByte(GenericDaoBaseTest.java:144)
  2. -> at com.cloud.utils.db.GenericDaoBaseTest.getObjectPrimitiveLong(GenericDaoBaseTest.java:117)
  3. -> at com.cloud.utils.db.GenericDaoBaseTest.getObjectPrimitiveInt(GenericDaoBaseTest.java:126)
  4. -> at com.cloud.utils.db.GenericDaoBaseTest.getObjectPrimitiveDouble(GenericDaoBaseTest.java:90)
  5. -> at com.cloud.utils.db.GenericDaoBaseTest.getObjectPrimitiveFloat(GenericDaoBaseTest.java:81)
  6. -> at com.cloud.utils.db.GenericDaoBaseTest.getObjectPrimitiveShort(GenericDaoBaseTest.java:54)
  7. -> at com.cloud.utils.db.GenericDaoBaseTest.getObjectPrimitiveBoolean(GenericDaoBaseTest.java:45)
Please remove unnecessary stubbings or use 'lenient' strictness. More info: javadoc for UnnecessaryStubbingException class.

	at org.mockito.internal.runners.StrictRunner.run(StrictRunner.java:49)
	at org.mockito.junit.MockitoJUnitRunner.run(MockitoJUnitRunner.java:163)
	at org.mockito.runners.MockitoJUnitRunner.run(MockitoJUnitRunner.java:54)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:69)
	at com.intellij.rt.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:33)
	at com.intellij.rt.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:220)
	at com.intellij.rt.junit.JUnitStarter.main(JUnitStarter.java:53)

* @throws EntityExistsException
*/
protected static void handleEntityExistsException(SQLException e) throws EntityExistsException {
boolean isIntegrityConstantViolation = INTEGRITY_CONSTRAINT_VIOLATION.equals(e.getSQLState());
Copy link
Member

@yadvr yadvr Aug 15, 2020

Choose a reason for hiding this comment

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

+1 to this pattern to check contant.equals(potentially-nullable-obj)

@yadvr
Copy link
Member

yadvr commented Aug 15, 2020

@blueorangutan package

@blueorangutan
Copy link

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

Packaging result: ✔centos7 ✔debian. JID-1762

@yadvr
Copy link
Member

yadvr commented Aug 15, 2020

@blueorangutan test

@blueorangutan
Copy link

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

Trillian test result (tid-2465)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 45651 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4268-t2465-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 81 look OK, 4 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
test_01_deploy_kubernetes_cluster Error 0.07 test_kubernetes_clusters.py
test_02_deploy_kubernetes_ha_cluster Error 0.03 test_kubernetes_clusters.py
test_04_deploy_and_upgrade_kubernetes_cluster Error 0.03 test_kubernetes_clusters.py
test_05_deploy_and_upgrade_kubernetes_ha_cluster Error 0.03 test_kubernetes_clusters.py
test_06_deploy_and_invalid_upgrade_kubernetes_cluster Error 0.03 test_kubernetes_clusters.py
test_07_deploy_and_scale_kubernetes_cluster Error 0.03 test_kubernetes_clusters.py
test_01_add_delete_kubernetes_supported_version Error 1807.10 test_kubernetes_supported_versions.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL Failure 735.81 test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers Failure 314.42 test_vpc_redundant.py
test_hostha_kvm_host_fencing Error 244.55 test_hostha_kvm.py

@yadvr yadvr requested a review from DaanHoogland August 17, 2020 08:32
Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

looks good, trillian failures are unrelated afaics.

@yadvr yadvr merged commit 3fe724b into apache:master Aug 17, 2020
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.

4 participants