-
Notifications
You must be signed in to change notification settings - Fork 25
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: Check new license and alternative license names in OSORI DB #514
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #514 +/- ##
=======================================
Coverage ? 91.34%
Complexity ? 517
=======================================
Files ? 48
Lines ? 1733
Branches ? 209
=======================================
Hits ? 1583
Misses ? 83
Partials ? 67 ☔ View full report in Codecov by Sentry. |
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.
Thank you for opening the PR.
It seems very good to bring OSORI DB, however it's too big to review in a PR.
Can you break it down into the separate PRs?
For example, I think you can make a PR for changing LPVSWebhookUtil
to LPVSPayloadUtil
.
566068b
to
3c0d510
Compare
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 take a look at my comments. Many thanks!
public LPVSLicense getLicenseBySpdxIdAndName( | ||
String licenseSpdxId, Optional<String> licenseName) { | ||
String licName = licenseName.orElse(licenseSpdxId); | ||
// check by license SPDX ID | ||
LPVSLicense lic = findLicenseBySPDX(licenseSpdxId); | ||
if (null == lic) { | ||
// check by license name and alternative names | ||
lic = findLicenseByName(licName); | ||
} | ||
// create new license | ||
if (lic == null) { | ||
|
||
if (osoriDbUrl != null && !osoriDbUrl.trim().isEmpty()) { | ||
// check OSORI DB and try to find the license there | ||
try { | ||
HttpURLConnection connection = | ||
osoriConnection.createConnection(osoriDbUrl, licenseSpdxId); | ||
connection.setRequestMethod("GET"); | ||
connection.connect(); | ||
|
||
if (connection.getResponseCode() != 200) { | ||
throw new Exception( | ||
"HTTP error code (" | ||
+ connection.getResponseCode() | ||
+ "): " | ||
+ connection.getResponseMessage()); | ||
} | ||
|
||
BufferedReader in = | ||
LPVSPayloadUtil.createBufferReader( | ||
LPVSPayloadUtil.createInputStreamReader( | ||
connection.getInputStream())); | ||
String inputLine; | ||
StringBuffer response = new StringBuffer(); | ||
|
||
while ((inputLine = in.readLine()) != null) { | ||
response.append(inputLine); | ||
} | ||
in.close(); | ||
|
||
// if found, create new license with field values taken from OSORI DB | ||
lic = LPVSPayloadUtil.convertOsoriDbResponseToLicense(response.toString()); | ||
} catch (Exception e) { | ||
log.error("Error connecting OSORI DB: " + e.getMessage()); | ||
} | ||
} | ||
|
||
if (lic == null) { | ||
// if not found, create new license with default field values | ||
lic = new LPVSLicense(); | ||
lic.setSpdxId(licenseSpdxId); | ||
lic.setLicenseName(licName); | ||
lic.setAlternativeNames(null); | ||
lic.setAccess("UNREVIEWED"); | ||
} | ||
// save new license and add it to the license list | ||
lic = lpvsLicenseRepository.saveAndFlush(lic); | ||
} | ||
addLicenseToList(lic); | ||
return lic; | ||
} |
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.
What about refactoring the code in terms of the improvement points below?
- Improvement Points:
- The code is quite long and can be simplified by breaking it down into smaller methods.
- The code is checking for the license in multiple places, which can be refactored into a single method.
Please refer to the example code below and check if it is possible. 😁
public LPVSLicense getLicenseBySpdxIdAndName(String licenseSpdxId, Optional<String> licenseName) {
String licName = licenseName.orElse(licenseSpdxId);
// Check if the license exists in the database
LPVSLicense lic = findLicense(licenseSpdxId, licName);
// If not found, check OSORI DB and create a new license if found
if (lic == null) {
lic = findLicenseInOsoriDb(licenseSpdxId);
}
// If still not found, create a new license with default field values
if (lic == null) {
lic = createNewLicense(licenseSpdxId, licName);
}
// Save the license and add it to the license list
lic = lpvsLicenseRepository.saveAndFlush(lic);
return lic;
}
private LPVSLicense findLicense(String licenseSpdxId, String licName) {
LPVSLicense lic = findLicenseBySPDX(licenseSpdxId);
if (lic == null) {
lic = findLicenseByName(licName);
}
return lic;
}
private LPVSLicense findLicenseInOsoriDb(String licenseSpdxId) {
if (osoriDbUrl == null || osoriDbUrl.trim().isEmpty()) {
return null;
}
try {
HttpURLConnection connection = osoriConnection.createConnection(osoriDbUrl, licenseSpdxId);
connection.setRequestMethod("GET");
connection.connect();
if (connection.getResponseCode()!= 200) {
throw new Exception("HTTP error code (" + connection.getResponseCode() + "): " + connection.getResponseMessage());
}
String response = LPVSPayloadUtil.convertInputStreamToString(connection.getInputStream());
return LPVSPayloadUtil.convertOsoriDbResponseToLicense(response);
} catch (Exception e) {
log.error("Error connecting OSORI DB: " + e.getMessage());
return null;
}
}
private LPVSLicense createNewLicense(String licenseSpdxId, String licName) {
LPVSLicense lic = new LPVSLicense();
lic.setSpdxId(licenseSpdxId);
lic.setLicenseName(licName);
lic.setAlternativeNames(null);
lic.setAccess("UNREVIEWED");
return lic;
}
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.
@tiokim Thank you for the suggestion. I will apply the proposed changes in the current PR.
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.
Thank you for modifying it according to the review comments. I think the test code is also well written. One question is, is there a reason why the functions were public but not private?
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 make these functions public for providing better testability and cover them with unit tests (to avoid the usage of reflection to get a reference to the private methods). If you have other opinion, please let me know.
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.
LGTM Thanks for your contribution! 👍🏼
Signed-off-by: Oleg Kopysov <o.kopysov@samsung.com>
Signed-off-by: Oleg Kopysov <o.kopysov@samsung.com>
Signed-off-by: Oleg Kopysov <o.kopysov@samsung.com>
Pull Request
Description
Current PR contains integration with OSORI DB for checking license information based on license SPDX ID, name or alternative name.
Type of change
Please delete options that are not relevant.
Testing
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Test Configuration:
Checklist: