engine/schema: fix findActiveAccountById in AccountDaoImpl.java

[weizhouapache]

Description

This PR fixes an issue with findActiveAccountById in AccountDaoImpl.java

This method is used in some processes. The impact is unknown.

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)

Feature/Enhancement Scale or Bug Severity

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

How Has This Been Tested?

[yadvr]

@blueorangutan package

[blueorangutan]

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

[blueorangutan]

Packaging result: ✖️ el7 ✖️ el8 ✖️ debian ✖️ suse15. SL-JID 1768

[yadvr]

cc @weizhouapache pl check for failures, I'll rekick pkging
@blueorangutan package

[blueorangutan]

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

[sureshanaparti]

code LGTM

[blueorangutan]

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1780

[yadvr]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

Trillian test result (tid-2585)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31440 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5704-t2585-kvm-centos7.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

[DaanHoogland]

@weizhouapache code looks ok, but what is the sense of searching for a bunch of fields and the id. The id is unique so this search makes no sense to me. Do you know how it is used and why it is a bug there?

[weizhouapache]

@weizhouapache code looks ok, but what is the sense of searching for a bunch of fields and the id. The id is unique so this search makes no sense to me. Do you know how it is used and why it is a bug there?

@DaanHoogland
as I said in description, the issue is with findActiveAccountById

cloudstack/engine/schema/src/main/java/com/cloud/user/dao/AccountDaoImpl.java

Lines 199 to 203 in 85373e6

	public Account findActiveAccountById(Long accountId, Long domainId) {
	SearchCriteria<AccountVO> sc = AllFieldsSearch.create("id", accountId);
	sc.setParameters("domainId", domainId);
	return findOneBy(sc);
	}

this method search records by id, but "id" is not supported in AllFieldsSearch.
therefore the result is same as (id/accountid is ignored)

    @Override
    public Account findActiveAccountById(Long accountId, Long domainId) {
        SearchCriteria<AccountVO> sc = AllFieldsSearch.create();
        sc.setParameters("domainId", domainId);
        return findOneBy(sc);
    }

[DaanHoogland]

@weizhouapache code looks ok, but what is the sense of searching for a bunch of fields and the id. The id is unique so this search makes no sense to me. Do you know how it is used and why it is a bug there?

@DaanHoogland as I said in description, the issue is with findActiveAccountById

cloudstack/engine/schema/src/main/java/com/cloud/user/dao/AccountDaoImpl.java

Lines 199 to 203 in 85373e6

	public Account findActiveAccountById(Long accountId, Long domainId) {
	SearchCriteria<AccountVO> sc = AllFieldsSearch.create("id", accountId);
	sc.setParameters("domainId", domainId);
	return findOneBy(sc);
	}

this method search records by id, but "id" is not supported in AllFieldsSearch. therefore the result is same as (id/accountid is ignored)

    @Override
    public Account findActiveAccountById(Long accountId, Long domainId) {
        SearchCriteria<AccountVO> sc = AllFieldsSearch.create();
        sc.setParameters("domainId", domainId);
        return findOneBy(sc);
    }

Ah, you are right. I was confusing SearchBuilder with SearchCriteria.
Nowhere else in the file the id is used in conjunction with the AllFieldsSearch so I think we are good.

[weizhouapache]

Merged based on 2 approvals and trillian test result