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

Do not fail hard if an ORT result contains no analyzer result #3878

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

sschuberth
Copy link
Member

Pragmatically, when being asked to operate on empty input, then doing
nothing is the right thing to do, esp. in library code where the caller
should implement custom error handling.

However, as it is rather unusual to not even have an analyzer result in a
ORT result, still emit a warning in that case.

Signed-off-by: Sebastian Schuberth sebastian.schuberth@bosch.io

@oheger-bosch
Copy link
Member

Such empty input files are indeed a strange corner case. Logging a warning in this case makes certainly sense.

I was just thinking whether a tool running on such input should actually do nothing or whether it should at least produce an empty result of its own type. Then it would be possible to distinguish whether specific tools have been run on a concrete ORT result or not.

@sschuberth
Copy link
Member Author

or whether it should at least produce an empty result of its own type.

Given that we agree that ORT result files without an analyzer result are a corner cases, I'm not actually feeling comfortable with deliberately creating more such corner cases...

@oheger-bosch
Copy link
Member

How could an ORT result without an analyzer section be created in practice? Is this just a theoretical case? If so, I would even say it is reasonable to ignore the case at all. (And if anybody does strange things, they will get what they deserve.)

Or, as an alternative approach: Would it be possible to make the analyzer field in the ORT result mandatory?

@sschuberth
Copy link
Member Author

How could an ORT result without an analyzer section be created in practice?

Currently, no ORT cli tool would write out an ORT result file without an analyzer result. I'm not sure about the helper-cli tools, though, e.g. when fed with "odd" options, or your new JSLT tool.

Would it be possible to make the analyzer field in the ORT result mandatory?

That's worth a thought. Although I currently like the fact that data-model-wise all results are nullable, and are treated the same.

"The provided ORT result file '${ortFile.canonicalPath}' does not contain an analyzer result."
if (ortResult.analyzer == null) {
log.warn {
"The provided ORT result file '${ortFile.canonicalPath}' does not contain an analyzer result."
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be worth mentioning in the warning that this will lead to no vulnerability information being retrieved/ an unchanged OrtResult?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@mnonnenmacher
Copy link
Member

mnonnenmacher commented Apr 14, 2021

Pragmatically, when being asked to operate on empty input, then doing
nothing is the right thing to do, esp. in library code where the caller
should implement custom error handling.

I don't understand that sentence, if with this change there is no exception anymore, how is the caller supposed to implement custom error handling?
If this code is used as a library, and for example the caller passed an ORT result with only scan results because they expect it to work, wouldn't it be easier for the user to find the issue if an exception is thrown, instead of having to notice the warning in the logs?

@sschuberth
Copy link
Member Author

sschuberth commented Apr 14, 2021

I don't understand that sentence, if with this change there is no exception anymore, how is the caller supposed to implement custom error handling?

I was meaning to say that, if the caller does want to handle empty files differently, it should check whether the file it empty before calling the function.

If this code is used as a library, and for example the caller passed an ORT result with only scan results because they expect it to work, wouldn't it be easier for the user to find the issue if an exception is thrown, instead of having to notice the warning in the logs?

Probably yes, but the caller would still need to (programmatically) inspect the exception message to find out whether it was thrown due to a missing analyzer result or something else. Also no nice.

@mnonnenmacher, what do you think about @oheger-bosch's suggestion to instead make the analyzer result non-nullable?

Pragmatically, when being asked to operate on empty input, then doing
nothing is the right thing to do, esp. in library code where the caller
should implement custom error handling.

However, as it is rather unusual to not even have an analyzer result in a
ORT result, still emit a warning in that case.

Signed-off-by: Sebastian Schuberth <sebastian.schuberth@bosch.io>
@sschuberth sschuberth enabled auto-merge (rebase) April 19, 2021 14:28
@sschuberth sschuberth merged commit ab9d20c into master Apr 19, 2021
@sschuberth sschuberth deleted the only-warn-no-analyzer branch April 19, 2021 15:52
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.

5 participants