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

Update java tools with methods to be called from online tool. #106

Merged
merged 6 commits into from
Aug 8, 2017

Conversation

rtgdk
Copy link
Contributor

@rtgdk rtgdk commented Aug 6, 2017

I have added methods somewhat similar to main methods of different tools but with fewer repetition of calling the same function from Python. Some of the methods returns warning/error message which can be displayed to the user. Also, I have removed any System.exit part in the method and rather thrown an OnlineToolException for every error that occur in the method. This can be caught by Jpype.
This way the JVM won't shutdown causing Django app to not shutdown inappropriately, Most of the time OnlineToolException won't be called as file exist and file check happens in python only but still I have added throw to them.

I hope this patch will ensure that System.exit is never encountered and the online app is run smoothly. If any of the internal methods are causing System.exit, I can later add SecurityManager to these methods and catch SecurityException.

…ess repetition of same function being called multiple times from python code

Signed-off-by: rtgdk <rohit.lodhartg@gmail.com>
Signed-off-by: rtgdk <rohit.lodhartg@gmail.com>
… retval, instead return verify only

Signed-off-by: rtgdk <rohit.lodhartg@gmail.com>
…ective conversion onlineFunction method so there is no need to call verifyRDFFile for each of the RDF file before passing it to them.

	* Removes repetition of code

Signed-off-by: rtgdk <rohit.lodhartg@gmail.com>
…oSpreadsheet conversion onlineFunction method so there is no need to call verifyTagFile for each of the Tag Value file before passing it to them.

        * Removes repetition of code

Signed-off-by: rtgdk <rohit.lodhartg@gmail.com>
@rtgdk
Copy link
Contributor Author

rtgdk commented Aug 6, 2017

I have added verification check for RDF files in all the RDF Conversion. Now there is no need to call verifyRDFFile or verify separately. I found the code for the RDF conversion and verification similiar, so added some of the remaining part and merged the verfication code in the onlineFunction itself.
Also, verification for Tag Value file can also happen in TagToSpreadsheet onlineFunction so no need to verify file before passing it to the function.
I am still struggling to understand the difference between the output of convertTagFileToRdf ( TagToRDF Line no.250 ) and convertTagFileToRdf( CompareSPDXDocs line no. 1191 ) . Please help me with this. Once this is done, all the Tag Value File Conversion would not need the verifyTagFile method to called separately before calling the conversion onlineFunction as the verification of the file can happen in the later function only.

PS : There is still some work left - Verification in Spreadsheet to Other conversion and in CompareMultipleDocs . I have no clue how the excel file is verified right now. I'll go through the code and make a separate PR for that. And for CompareMultipleDocs, I think it would be better to call the verify function on each of the files before sending those to CompareMultipleDocs, as the online tool would show error for all the inputted files. I don't want the method to fail if the first file is wrong. I want all the files to be verifies and show the user all the errors in all the files. So VerifyRDFFile method is good for that.

@goneall
Copy link
Member

goneall commented Aug 7, 2017

@rtgdk I can see the issue with calling the main method directly as it is designed to be used as a command line utility and there are scripts which depend on the System.exit being called to properly set the exit code. Making a copy of the main method will cause some additional maintenance effort as we now have 2 copies of the code we need to update if there is a fix or enhancement. Rather than making a copy for the online tool access, I would recommend creating changing the main methods to call the onlineFunction and catch the exception. On catching the exception, the main method could System.print the exception message and System.exit(ERROR_CODE).

@goneall
Copy link
Member

goneall commented Aug 7, 2017

@rtgdk Any System.exit calls will be within the tool utility class itself (e.g. TagToSpreadsheet.java). In most cases, they will just be in the main method. You should not find any System.exit calls for any methods called for any other classes.

@goneall
Copy link
Member

goneall commented Aug 7, 2017

I am still struggling to understand the difference between the output of convertTagFileToRdf ( TagToRDF Line no.250 ) and convertTagFileToRdf( CompareSPDXDocs line no. 1191 ) .

The only difference is the CompareSPDXDocs takes a File as an input and TagToRDF takes an input stream. The CompareSPDXDocs method calls the TagToRDF convertTagFileToRdf method after opening the file.

The code is not well structured, the compareSPDXDocs convertTagFileToRdf should be moved to the TagToRDF file to keep all of the TagtoRdf methods together in one logical place.

I do recall that the TagToRDF.convertTagFileToRdf was created so that tools and utilities could call the method directly without having to go through the main method (avoiding the System.exit problem).

@goneall
Copy link
Member

goneall commented Aug 7, 2017

@rtgdk The Excel verification is done in the method SPDXSpreadsheet.verifyWorkbook which is called when the spreadsheet is constructed.

…intaining copies of the same function

Signed-off-by: rtgdk <rohit.lodhartg@gmail.com>
@rtgdk
Copy link
Contributor Author

rtgdk commented Aug 7, 2017

The Excel verification is done in the method SPDXSpreadsheet.verifyWorkbook which is called when the spreadsheet is constructed.

Thanks. I saw the flow of the Spreadsheet conversion code. No need for a separate function to validate Spreadsheet since they are only input in conversion and proper error are thrown to the online tool.

I'll push the changes you suggested ( calling onlineFunction from main ) in a while. Some of the tool don't call System.exit in their main function. So I'll just call the usage() function there and return.

@rtgdk
Copy link
Contributor Author

rtgdk commented Aug 7, 2017

Any System.exit calls will be within the tool utility class itself (e.g. TagToSpreadsheet.java). In most cases, they will just be in the main method. You should not find any System.exit calls for any methods called for any other classes.

Good then we won't need SecurityManager for the onlineFunction.

@rtgdk
Copy link
Contributor Author

rtgdk commented Aug 7, 2017

About that TagToRDF.convertTagFileToRdf, I was worried about the handling of different exception thrown by convertTagFileToRdf(InputStream spdxTagFile, String outputFormat, List<String> warnings). I later realised that the exception will be pushed up to the calling chain and it will ultimately be catched by the onlineFunction. So no need to worry about the verification. We are good to go in TagToRDF as well.

@goneall goneall merged commit 5b82195 into spdx:master Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants