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

False InvalidLicenseChoiceException for complex expression #8082

Closed
mmurto opened this issue Jan 4, 2024 · 4 comments
Closed

False InvalidLicenseChoiceException for complex expression #8082

mmurto opened this issue Jan 4, 2024 · 4 comments
Labels
bug Issues that are considered to be bugs spdx-utils About the SPDX utility library

Comments

@mmurto
Copy link
Contributor

mmurto commented Jan 4, 2024

Expression (MIT OR GPL-2.0-only) AND (MIT OR BSD-3-Clause OR GPL-1.0-or-later) AND (MIT OR BSD-3-Clause OR GPL-2.0-only) doesn't allow choice of MIT AND MIT AND MIT, but throws the following exception:

Caused by: org.ossreviewtoolkit.utils.spdx.InvalidLicenseChoiceException: MIT AND MIT AND MIT is not a valid choice for (MIT OR GPL-2.0-only) AND (MIT OR BSD-3-Clause OR GPL-1.0-or-later) AND (MIT OR BSD-3-Clause OR GPL-2.0-only). Valid choices are: [MIT AND BSD-3-Clause AND GPL-1.0-or-later, MIT AND BSD-3-Clause AND GPL-1.0-or-later AND GPL-2.0-only].
@sschuberth sschuberth added bug Issues that are considered to be bugs spdx-utils About the SPDX utility library labels Jan 4, 2024
@sschuberth
Copy link
Member

The bug seems to be in validChoicesForDnf(), not in disjunctiveNormalForm().

sschuberth added a commit that referenced this issue Jan 11, 2024
@sschuberth
fix(spdx-utils): Correctly determine choices for nested AND express…
1a6c1bf 
…ions

The original code was clearly wrong in that it recursively decomposed an
expression and composed the contained expressions always with `AND`
although there could be nested `OR` expressions.

Instead, if there is a top-level `AND` in an expressions, create all
combinations of choices on the left and choices on the right to get the
total valid choices.

Fixes #8082.

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit that referenced this issue Jan 11, 2024
@sschuberth
fix(spdx-utils): Correctly determine choices for nested AND express…
14c970e 
…ions

The original code was clearly wrong in that it recursively decomposed an
expression and composed the contained expressions always with `AND`
although there could be nested `OR` expressions.

Instead, if there is a top-level `AND` in an expressions, create all
combinations of choices on the left and choices on the right to get the
total valid choices.

Fixes #8082.

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit that referenced this issue Jan 11, 2024
@sschuberth
fix(spdx-utils): Correctly determine choices for AND expressions
95fe263 
The original code was clearly wrong in that it recursively decomposed an
expression and composed the contained expressions always with `AND`
although there could be nested `OR` expressions.

Instead, if there is a top-level `AND` in an expressions, create all
combinations of choices on the left and choices on the right to get the
total valid choices.

Fixes #8082.

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth
Copy link
Member

@mmurto, I have a semi-off-topic question: Given (MIT OR GPL-2.0-only) AND (MIT OR BSD-3-Clause OR GPL-1.0-or-later) AND (MIT OR BSD-3-Clause OR GPL-2.0-only), would you also expect / wish for MIT to be a valid choice, or is the explicit MIT AND MIT AND MIT ok / desired?

@mmurto
Copy link
Contributor Author

mmurto commented Jan 12, 2024

would you also expect / wish for MIT to be a valid choice, or is the explicit MIT AND MIT AND MIT ok / desired?

Personally I'd prefer MIT there, but MIT AND MIT AND MIT is totally ok to write in the configuration.

@sschuberth sschuberth mentioned this issue Jan 12, 2024
Merged
sschuberth added a commit that referenced this issue Jan 12, 2024
@sschuberth
test(spdx-utils): Add a test for a complex license choice
5923add 
See [1] for context.

[1]: #8082 (comment)

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit that referenced this issue Jan 12, 2024
@sschuberth
fix(spdx-utils): Correctly determine choices for AND expressions
541a9a8 
The original code was clearly wrong in that it recursively decomposed an
expression and composed the contained expressions always with `AND`
although there could be nested `OR` expressions.

Instead, if there is a top-level `AND` in an expressions, create all
combinations of choices on the left and choices on the right to get the
total valid choices.

Fixes #8082.

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit that referenced this issue Jan 12, 2024
@sschuberth
test(spdx-utils): Add a test for a complex license choice
2fed658 
See [1] for context.

[1]: #8082 (comment)

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit that referenced this issue Jan 12, 2024
@sschuberth
test(spdx-utils): Add a test for a complex license choice
854b7af 
See [1] for context.

[1]: #8082 (comment)

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit that referenced this issue Jan 15, 2024
@sschuberth
fix(spdx-utils): Correctly determine choices for AND expressions
7f10104 
The original code was clearly wrong in that it recursively decomposed an
expression and composed the contained expressions always with `AND`
although there could be nested `OR` expressions.

Instead, if there is a top-level `AND` in an expressions, create all
combinations of choices on the left and choices on the right to get the
total valid choices.

Fixes #8082.

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit that referenced this issue Jan 15, 2024
@sschuberth
test(spdx-utils): Add a test for a complex license choice
3892bd3 
See [1] for context.

[1]: #8082 (comment)

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit that referenced this issue Jan 17, 2024
@sschuberth
fix(spdx-utils): Correctly determine choices for AND expressions
8ac2004 
The original code was clearly wrong in that it recursively decomposed an
expression and composed the contained expressions always with `AND`
although there could be nested `OR` expressions.

Instead, if there is a top-level `AND` in an expressions, create all
combinations of choices on the left and choices on the right to get the
total valid choices.

Fixes #8082.

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit that referenced this issue Jan 17, 2024
@sschuberth
test(spdx-utils): Add a test for a complex license choice
7ad0a64 
See [1] for context.

[1]: #8082 (comment)

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit that referenced this issue Jan 17, 2024
@sschuberth
fix(spdx-utils): Correctly determine choices for AND expressions
6ad4d0a 
The original code was clearly wrong in that it recursively decomposed an
expression and composed the contained expressions always with `AND`
although there could be nested `OR` expressions.

Instead, if there is a top-level `AND` in an expressions, create all
combinations of choices on the left and choices on the right to get the
total valid choices.

Reword affected tests according to the fact that returned choices are
not simplified, but explicit, though still deduplicated. Commonly
introduce a `choice` variable along the way to make tests for long
license expressions more readable.

Fixes #8082.

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit that referenced this issue Jan 17, 2024
@sschuberth
test(spdx-utils): Add a test for a complex license choice
c787f7a 
See [1] for context.

[1]: #8082 (comment)

Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth added a commit that referenced this issue Jan 18, 2024
@sschuberth
test(spdx-utils): Add a test for a complex license choice
34046a6 
See [1] for context.

[1]: #8082 (comment)

Signed-off-by: Sebastian Schuberth <[email protected]>
@sschuberth
Copy link
Member

Personally I'd prefer MIT there, but MIT AND MIT AND MIT is totally ok to write in the configuration.

I've verified that just using "MIT" in this case is actually already working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Issues that are considered to be bugs spdx-utils About the SPDX utility library
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@sschuberth @mmurto