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

feat(couchdb-java-client): Migrating from ektorp to cloudant #1149

Merged
merged 2 commits into from
May 7, 2021

Conversation

smrutis1
Copy link
Contributor

Please provide a summary of your changes here.

Have added a new library(cloudant java client) instead of ektorp for DB operations.

Issue: Closes #1148

Suggest Reviewer

You can suggest reviewers here with an @mention.

How To Test?

Need to

Have you implemented any additional tests? - No

Checklist

Must:

  • All related issues are referenced in commit messages and in PR

Signed-off-by: Smruti Prakash Sahoo smruti.sahoo@siemens.com

@mcjaeger
Copy link
Contributor

test run cleanly on vagrant (Java 11 based, Ubuntu 20, Couchdb 311)

@smrutis1 smrutis1 added needs code review needs general test This is general testing, meaning that there is no org specific issue to check for and removed WIP work in progress labels Apr 20, 2021
Copy link
Contributor

@JaideepPalit JaideepPalit left a comment

Choose a reason for hiding this comment

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

Reviewed the PR.
Kindly have a look at the review comments.

List<DocumentOperationResult> results = attachmentUsageRepository.executeBulk(sanitizedUsages);
if (!results.isEmpty()) {
List<Response> results = attachmentUsageRepository.executeBulk(sanitizedUsages);
if (CommonUtils.isNotEmpty(sanitizedUsages) && results.isEmpty()) {
throw new SW360Exception("Some of the usage documents could not be created: " + results);
Copy link
Contributor

Choose a reason for hiding this comment

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

results seems to be empty , maybe it would be good to change exception message as well since previously exception was thrown when not empty
same for line 201

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have validated the response/results before exception

@@ -185,20 +184,20 @@ public AttachmentUsage updateAttachmentUsage(AttachmentUsage attachmentUsage) {
}

public void updateAttachmentUsages(List<AttachmentUsage> attachmentUsages) throws TException {
List<DocumentOperationResult> results = attachmentUsageRepository.executeBulk(attachmentUsages);
List<Response> results = attachmentUsageRepository.executeBulk(attachmentUsages);
Copy link
Contributor

Choose a reason for hiding this comment

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

From the documentation, it specifies, that for each record/request there is a response and that response should be validated with statusCode and getError() to ensure no failed records before the next check in line 188-190.

Based on java docs from com.cloudant.client.api.database.class

     * <p>
     * Note that the value returned by {@code getStatusCode()} on each {@code Response} object is
     * the overall status returned from {@code bulk_docs} and will therefore be the same for all
     * {@code Response} objects.
     * </p>
     * <p>
     * The returned list of {@code Response}s should be examined to ensure that all of the documents submitted
     * in the original request were successfully added to the database.
     * </p>
     * <p>
     * When a document (or document revision) is not correctly committed to the database because of
     * an error, you must check the value of {@code getError()} to determine error type and course
     * of action. See
     * <a href="https://console.bluemix.net/docs/services/Cloudant/api/document.html#bulk-document-validation-and-conflict-errors" target="_blank">
     * Bulk Document Validation and Conflict Errors</a> for more information.
     * </p>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added the validation

}

@View(name = "projectVulnerabilityRating", map = PROJECT_VULNERABILITY_LINK)
public List<ProjectVulnerabilityRating> getProjectVulnerabilityRating(String projectId) {
List<ProjectVulnerabilityRating> queryResults = queryByPrefix("projectVulnerabilityRating", projectId);

if (queryResults.size() > 1) {
log.error("More than one projectVulnerabilityRating found for project with id " + projectId);
// log.error("More than one projectVulnerabilityRating found for project with id " + projectId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove/uncomment commented line of code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added the logger

@@ -478,7 +482,7 @@ public RequestStatus updateLicenseFromAdditionsAndDeletions(License licenseAddit
if (!PermissionUtils.isUserAtLeast(UserGroup.CLEARING_ADMIN, user)){
return null;
}
final List<DocumentOperationResult> documentOperationResults = licenseTypeRepository.executeBulk(licenseTypes);
final List<Response> documentOperationResults = licenseTypeRepository.executeBulk(licenseTypes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Kindly check whether returned Response needs to be validated for any error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have added the validation

@JaideepPalit JaideepPalit added needs rework This doesn't seem right after review and removed needs code review labels Apr 22, 2021
@ravi110336
Copy link
Contributor

ravi110336 commented Apr 22, 2021

Tested the UI and it is working fine for most of the cases but found some below issue, Please check:

Issue 1:
Add release to a component and while fetching the release details getting the error message " Error:Error fetching release from backend." ---> Working fine.
image

Issue 2:
Similar issue from Admin->DataBaseSanitation->Releases with more than one source attachment:
getting the error message " Error:Error fetching release from backend." --->Working fine.

Issue 3:
ECC-> click on release. getting the error message " Error:Error fetching release from backend." --->Working fine.

Observation 1: (Not sure if this is an issue )
Go to Requests -> click on any request -> blank result no output. ---> Working fine.
image

@smrutis1 smrutis1 force-pushed the feat/CloudantMigrate branch 3 times, most recently from 36f301e to b4d3d90 Compare April 23, 2021 05:33
@smrutis1
Copy link
Contributor Author

@JaideepPalit , Thanks for the review. Have incorporated most of them. Kindly have a look and let me know. @ravi110336 Thanks for the test. Have fixed the mentioned issues however the observation regarding "Request" tab, I am not able to reproduce it.

@smrutis1 smrutis1 added needs code review and removed needs rework This doesn't seem right after review labels Apr 23, 2021
@smrutis1 smrutis1 force-pushed the feat/CloudantMigrate branch 3 times, most recently from cf7b624 to 466a359 Compare April 30, 2021 06:48
@smrutis1
Copy link
Contributor Author

@ravi110336 , From discussion, have rectified the issue with moderation request. Kindly have a look

@ravi110336
Copy link
Contributor

Tested the moderation request and attachment download working fine.

@mcjaeger
Copy link
Contributor

tested also with vagrant: looks good

Copy link
Contributor

@JaideepPalit JaideepPalit left a comment

Choose a reason for hiding this comment

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

Re-reviewed the PR.
Kindly have a look at some very minor comments.

Note-
The travis-ci logs gives lot of error messages during test, which are not coming in other branches.
https://travis-ci.org/github/eclipse/sw360/jobs/768972323#L2046

Apr 30, 2021 6:56:28 AM com.cloudant.client.org.lightcouch.CouchDbClient createDB
INFO: Created Database: 'sw360_test_db'
2021-04-30 06:56:28 ERROR DatabaseConnectorCloudant:138 - Error fetching document of type DesignDocument with id _design/AttachmentUsage : 404 Object Not Found at http://localhost:5984/sw360_test_db/_design/AttachmentUsage. Error: not_found. Reason: missing.
2021-04-30 06:56:28 ERROR DatabaseConnectorCloudant:138 - Error fetching document of type DesignDocument with id _design/Attachment : 404 Object Not Found at http://localhost:5984/sw360_test_db/_design/Attachment. Error: not_found. Reason: missing.
2021-04-30 06:56:28 ERROR DatabaseConnectorCloudant:138 - Error fetching document of type DesignDocument with id _design/Source : 404 Object Not Found at http://localhost:5984/sw360_test_db/_design/Source. Error: not_found. Reason: missing.
2021-04-30 06:56:28 ERROR DatabaseConnectorCloudant:138 - Error fetching document of type DesignDocument with id _design/Vendor : 404 Object Not Found at http://localhost:5984/sw360_test_db/_design/Vendor. Error: not_found. Reason: missing.
2021-04-30 06:56:28 ERROR DatabaseConnectorCloudant:138 - Error fetching document of type DesignDocument with id _design/Release : 404 Object Not Found at http://localhost:5984/sw360_test_db/_design/Release. Error: not_found. Reason: missing.
2021-04-30 06:56:29 ERROR DatabaseConnectorCloudant:138 - Error fetching document of type DesignDocument with id _design/Component : 404 Object Not Found at http://localhost:5984/sw360_test_db/_design/Component. Error: not_found. Reason: missing.
2021-04-30 06:56:29 ERROR DatabaseConnectorCloudant:138 - Error fetching document of type DesignDocument with id _design/Project : 404 Object Not Found at http://localhost:5984/sw360_test_db/_design/Project. Error: not_found. Reason: missing.
2021-04-30 06:56:29 ERROR DatabaseConnectorCloudant:138 - Error fetching document of type DesignDocument with id _design/User : 404 Object Not Found at http://localhost:5984/sw360_test_db/_design/User. Error: not_found. Reason: missing.
2021-04-30 06:56:29 ERROR DatabaseConnectorCloudant:138 - Error fetching document of type DesignDocument with id _design/ProjectVulnerabilityRating : 404 Object Not Found at http://localhost:5984/sw360_test_db/_design/ProjectVulnerabilityRating. Error: not_found. Reason: missing.
2021-04-30 06:56:29 ERROR DatabaseConnectorCloudant:138 - Error fetching document of type DesignDocument with id 

Seems like , using cloudant - design docs are not getting generated.
It should also be tested on a clean system with no DBs or Design docs

@JaideepPalit JaideepPalit removed needs code review needs general test This is general testing, meaning that there is no org specific issue to check for labels Apr 30, 2021
@JaideepPalit JaideepPalit added the needs rework This doesn't seem right after review label Apr 30, 2021
@ravi110336
Copy link
Contributor

I have tested the branch after deleting all the DBs and design docs in my local system, all the DBs are getting created with the design docs successfully.

Signed-off-by: Smruti Prakash Sahoo <smruti.sahoo@siemens.com>
@smrutis1
Copy link
Contributor Author

smrutis1 commented May 6, 2021

@JaideepPalit Thanks for review. Have incorporated changes.

Note-
The travis-ci logs gives lot of error messages during test, which are not coming in other branches.
https://travis-ci.org/github/eclipse/sw360/jobs/768972323#L2046

(from discussion): This is due to added logger message, it's ok to keep it.

  • Have added the paginated view for components to mitigate loading issue

@smrutis1 smrutis1 added needs code review needs general test This is general testing, meaning that there is no org specific issue to check for and removed needs rework This doesn't seem right after review labels May 6, 2021
Copy link
Contributor

@JaideepPalit JaideepPalit left a comment

Choose a reason for hiding this comment

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

Reviewed the PR.
Kindly have a look at some review comments.
Also seems to be some issue with

  1. sorting based on vendor and main licenses

    • case-sensitivity
    • Multivalue list
      sort1
  2. There seems to some issue with pagination. Based on columns sorted like vendor/license/name - total result seems to vary
    comp_name
    main_lic

case 3:
query = getConnector().createQuery(Component.class, "bycomponenttype");
break;

Copy link
Contributor

Choose a reason for hiding this comment

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

Fix indentation for switch - case
extra newline could also be removed

Copy link
Contributor Author

@smrutis1 smrutis1 May 7, 2021

Choose a reason for hiding this comment

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

Removed new line. Tried with CTRL+SHIFT+F, didn't help with switch case indentation.

@JaideepPalit JaideepPalit added needs rework This doesn't seem right after review and removed needs code review needs general test This is general testing, meaning that there is no org specific issue to check for labels May 7, 2021
Signed-off-by: Smruti Prakash Sahoo <smruti.sahoo@siemens.com>
@smrutis1
Copy link
Contributor Author

smrutis1 commented May 7, 2021

Reviewed the PR.
Kindly have a look at some review comments.
Also seems to be some issue with

  1. sorting based on vendor and main licenses

    • case-sensitivity
    • Multivalue list\

(from discussion) Raised an issue #1199

  1. There seems to some issue with pagination. Based on columns sorted like vendor/license/name - total result seems to vary
    comp_name
    main_lic

Fixed it.

@smrutis1 smrutis1 added needs code review and removed needs rework This doesn't seem right after review labels May 7, 2021
Copy link
Contributor

@JaideepPalit JaideepPalit left a comment

Choose a reason for hiding this comment

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

Code looks fine to me.

@mcjaeger
Copy link
Contributor

mcjaeger commented May 7, 2021

has been tested on our staging server, vagrant of course and again staging server....

@mcjaeger mcjaeger merged commit a91c7a9 into eclipse-sw360:master May 7, 2021
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.

Migrating from Ektorp to Cloudant Java Client
4 participants