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.

Sign up for GitHub

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

Jump to bottom

[BISERVER-14935] import-export.bat/.sh does not work on Pentaho 9.3 or higher if requestParameterAuthenticationEnabled=true #5417

Merged
merged 3 commits into from
Nov 3, 2023
Merged

[BISERVER-14935] import-export.bat/.sh does not work on Pentaho 9.3 or higher if requestParameterAuthenticationEnabled=true #5417

merged 3 commits into from
Nov 3, 2023

Conversation

smmribeiro
Copy link
Contributor

@smmribeiro smmribeiro commented Oct 19, 2023 •
edited

Two commits on this PR: the first is the actual fix for this issue's problem; the second is a refactoring of the class to fix some sonar issues and some architectural problems found during the test and development of the first.

@bcostahitachivantara @renato-s @andreramos89

@smmribeiro smmribeiro requested a review from a team as a code owner October 19, 2023 08:39
@buildguy

This comment has been minimized.

Sign in to view
bcostahitachivantara
bcostahitachivantara reviewed Oct 23, 2023
View reviewed changes
...ns/src/main/java/org/pentaho/platform/plugin/services/importexport/CommandLineProcessor.java Outdated
String backupURL = contextURL + API_REPO_FILES_BACKUP;

// BISERVER-14935: In case "requestParameterAuthenticationEnabled" is set to 'true'
backupURL += ( "?userid=" + getUsername() + "&password=" + Encr.encryptPassword(
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you are creating a string on line 736 and then creating a new one. Why not create the string with all data instead of reassigning a new value or use a StringBuilder.

...ns/src/main/java/org/pentaho/platform/plugin/services/importexport/CommandLineProcessor.java Show resolved Hide resolved
...ns/src/main/java/org/pentaho/platform/plugin/services/importexport/CommandLineProcessor.java Outdated
@@ -346,10 +362,10 @@ private void performREST() throws ParseException, KettleException, URISyntaxExce
Messages.getInstance().getString( "CommandLineProcessor.INFO_REST_RESPONSE_STATUS", response.getStatus() );
message += "\n";

if ( logFile != null && !"".equals( logFile ) ) {
if ( logFile != null && !logFile.isEmpty() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not StringUtils.isEmpty( logFile )?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...ns/src/main/java/org/pentaho/platform/plugin/services/importexport/CommandLineProcessor.java Outdated
if ( logFile != null && !"".equals( logFile ) ) {
writeFile( message.toString(), logFile );
System.out.println( message );
if ( logFile != null && !logFile.isEmpty() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not StringUtils.isEmpty( logFile )?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...ns/src/main/java/org/pentaho/platform/plugin/services/importexport/CommandLineProcessor.java Outdated
message += Messages.getInstance().getString( "CommandLineProcessor.INFO_EXPORT_WRITTEN_TO", filename );
if ( logFile != null && !"".equals( logFile ) ) {
message += Messages.getInstance().getString( "CommandLineProcessor.INFO_EXPORT_WRITTEN_TO", outputFile );
if ( logFile != null && !logFile.isEmpty() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not StringUtils.isEmpty( logFile )?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...ns/src/main/java/org/pentaho/platform/plugin/services/importexport/CommandLineProcessor.java Outdated
message += Messages.getInstance().getString( "CommandLineProcessor.INFO_EXPORT_WRITTEN_TO", filename );
if ( logFile != null && !"".equals( logFile ) ) {
message += Messages.getInstance().getString( "CommandLineProcessor.INFO_EXPORT_WRITTEN_TO", outputFile );
if ( logFile != null && !logFile.isEmpty() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not StringUtils.isEmpty( logFile )?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...ns/src/main/java/org/pentaho/platform/plugin/services/importexport/CommandLineProcessor.java Outdated
}
}

if ( !isValid && logFile != null && !"".equals( logFile ) ) {
writeFile( "Invalid file-path:" + filePath, logFile );
if ( !isValid && logFile != null && !logFile.isEmpty() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not StringUtils.isEmpty( logFile )?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

...ns/src/main/java/org/pentaho/platform/plugin/services/importexport/CommandLineProcessor.java Outdated
if ( StringUtils.isEmpty( value ) ) {
// Is it required?
if ( required ) {
throw new ParseException( Messages.getInstance().getErrorString( "CommandLineProcessor.ERROR_0001_MISSING_ARG",
Copy link
Contributor

Choose a reason for hiding this comment

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

You are returning the same message and same exception. Could you rewrite it in a OR format?

Copy link
Contributor

Choose a reason for hiding this comment

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

Bruno has a point. We can join the two inner if's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

andreramos89
andreramos89 approved these changes Oct 25, 2023
View reviewed changes
...ns/src/main/java/org/pentaho/platform/plugin/services/importexport/CommandLineProcessor.java Outdated
if ( StringUtils.isEmpty( value ) ) {
// Is it required?
if ( required ) {
throw new ParseException( Messages.getInstance().getErrorString( "CommandLineProcessor.ERROR_0001_MISSING_ARG",
Copy link
Contributor

Choose a reason for hiding this comment

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

Bruno has a point. We can join the two inner if's

@smmribeiro
[BISERVER-14935] import-export.bat/.sh does not work on Pentaho 9.3 o…
c7b3f56 
…r higher if requestParameterAuthenticationEnabled=true
@smmribeiro
[BISERVER-14935] import-export.bat/.sh does not work on Pentaho 9.3 o…
83fc88c 
…r higher if requestParameterAuthenticationEnabled=true

Cleanup:
 - use of constants
 - extract methods
 - use of try-resource
 - remove old unused methods
 - clean exceptions
 - refactor code to write to file
@smmribeiro
[BISERVER-14935] import-export.bat/.sh does not work on Pentaho 9.3 o…
7116ba7 
…r higher if requestParameterAuthenticationEnabled=true

Address the comments on the PR review
@buildguy
Copy link
Collaborator

buildguy commented Nov 1, 2023

✅ Build finished in 42m 37s

Build command:

mvn clean verify -B -e -Daudit -amd -pl extensions

👌 All tests passed!

Tests run: 1693, Failures: 0, Skipped: 1    Test Results

ℹ️ This is an automatic message

@hitachivantarasonarqube HitachiVantaraSonarQube
Copy link

SonarQube Quality Gate

Quality Gate failed

Failed condition 18.1% 18.1% Coverage on New Code (is less than 80%)
Failed condition 4.2% 4.23% Duplicated Lines (%) on New Code (is greater than 3%)
0.0% 0.0% Security Hotspots Reviewed on New Code (is less than 100%)

See analysis details on SonarQube

bcostahitachivantara
bcostahitachivantara approved these changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@bcostahitachivantara bcostahitachivantara 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!

@smmribeiro smmribeiro merged commit db41ab6 into pentaho:master Nov 3, 2023
3 of 5 checks passed
@smmribeiro smmribeiro mentioned this pull request Nov 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcostahitachivantara bcostahitachivantara bcostahitachivantara approved these changes

@andreramos89 andreramos89 andreramos89 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@smmribeiro @buildguy @bcostahitachivantara @andreramos89