From a8343f7e690e64e9c7eaba7282fa43394dcb5aec Mon Sep 17 00:00:00 2001 From: "E. Lynette Rayle" Date: Mon, 20 Mar 2023 11:19:13 -0400 Subject: [PATCH 1/2] add failing tests for bug where GPL-2.0 failed to match GPL-2.0-only --- spdxexp/compare_test.go | 9 +++++++++ spdxexp/satisfies_test.go | 4 ++++ 2 files changed, 13 insertions(+) diff --git a/spdxexp/compare_test.go b/spdxexp/compare_test.go index 16f00d1..eb09e46 100644 --- a/spdxexp/compare_test.go +++ b/spdxexp/compare_test.go @@ -20,9 +20,12 @@ func TestCompareGT(t *testing.T) { {"expect greater than: AGPL-3.0 > AGPL-1.0", getLicenseNode("AGPL-3.0", false), getLicenseNode("AGPL-1.0", false), true}, {"expect equal: GPL-2.0-or-later > GPL-2.0-only", getLicenseNode("GPL-2.0-or-later", true), getLicenseNode("GPL-2.0-only", false), false}, {"expect equal: GPL-2.0-or-later > GPL-2.0", getLicenseNode("GPL-2.0-or-later", true), getLicenseNode("GPL-2.0", false), false}, + {"expect equal: GPL-2.0-only > GPL-2.0", getLicenseNode("GPL-2.0-only", false), getLicenseNode("GPL-2.0", false), false}, {"expect equal: GPL-3.0 > GPL-3.0", getLicenseNode("GPL-3.0", false), getLicenseNode("GPL-3.0", false), false}, + {"expect equal: MIT > MIT", getLicenseNode("MIT", false), getLicenseNode("MIT", false), false}, {"expect less than: MPL-1.0 > MPL-2.0", getLicenseNode("MPL-1.0", false), getLicenseNode("MPL-2.0", false), false}, {"incompatible: MIT > ISC", getLicenseNode("MIT", false), getLicenseNode("ISC", false), false}, + {"incompatible: MIT > GPL-2.0-only", getLicenseNode("MIT", false), getLicenseNode("GPL-2.0-only", false), false}, {"incompatible: OSL-1.0 > OPL-1.0", getLicenseNode("OSL-1.0", false), getLicenseNode("OPL-1.0", false), false}, {"not simple license: (MIT OR ISC) > GPL-3.0", getLicenseNode("(MIT OR ISC)", false), getLicenseNode("GPL-3.0", false), false}, // TODO: should it raise error? } @@ -49,9 +52,12 @@ func TestCompareEQ(t *testing.T) { {"expect greater than: AGPL-3.0 == AGPL-1.0", getLicenseNode("AGPL-3.0", false), getLicenseNode("AGPL-1.0", false), false}, {"expect equal: GPL-2.0-or-later > GPL-2.0-only", getLicenseNode("GPL-2.0-or-later", true), getLicenseNode("GPL-2.0-only", false), true}, {"expect equal: GPL-2.0-or-later > GPL-2.0", getLicenseNode("GPL-2.0-or-later", true), getLicenseNode("GPL-2.0", false), true}, + {"expect equal: GPL-2.0-only == GPL-2.0", getLicenseNode("GPL-2.0-only", false), getLicenseNode("GPL-2.0", false), true}, {"expect equal: GPL-3.0 == GPL-3.0", getLicenseNode("GPL-3.0", false), getLicenseNode("GPL-3.0", false), true}, + {"expect equal: MIT == MIT", getLicenseNode("MIT", false), getLicenseNode("MIT", false), true}, {"expect less than: MPL-1.0 == MPL-2.0", getLicenseNode("MPL-1.0", false), getLicenseNode("MPL-2.0", false), false}, {"incompatible: MIT == ISC", getLicenseNode("MIT", false), getLicenseNode("ISC", false), false}, + {"incompatible: MIT == GPL-2.0-only", getLicenseNode("MIT", false), getLicenseNode("GPL-2.0-only", false), false}, {"incompatible: OSL-1.0 == OPL-1.0", getLicenseNode("OSL-1.0", false), getLicenseNode("OPL-1.0", false), false}, {"not simple license: (MIT OR ISC) == GPL-3.0", getLicenseNode("(MIT OR ISC)", false), getLicenseNode("GPL-3.0", false), false}, // TODO: should it raise error? } @@ -78,9 +84,12 @@ func TestCompareLT(t *testing.T) { {"expect greater than: AGPL-3.0 < AGPL-1.0", getLicenseNode("AGPL-3.0", false), getLicenseNode("AGPL-1.0", false), false}, {"expect greater than: GPL-2.0-or-later < GPL-2.0-only", getLicenseNode("GPL-2.0-or-later", true), getLicenseNode("GPL-2.0-only", false), false}, {"expect greater than: GPL-2.0-or-later == GPL-2.0", getLicenseNode("GPL-2.0-or-later", true), getLicenseNode("GPL-2.0", false), false}, + {"expect equal: GPL-2.0-only < GPL-2.0", getLicenseNode("GPL-2.0-only", false), getLicenseNode("GPL-2.0", false), false}, {"expect equal: GPL-3.0 < GPL-3.0", getLicenseNode("GPL-3.0", false), getLicenseNode("GPL-3.0", false), false}, + {"expect equal: MIT < MIT", getLicenseNode("MIT", false), getLicenseNode("MIT", false), false}, {"expect less than: MPL-1.0 < MPL-2.0", getLicenseNode("MPL-1.0", false), getLicenseNode("MPL-2.0", false), true}, {"incompatible: MIT < ISC", getLicenseNode("MIT", false), getLicenseNode("ISC", false), false}, + {"incompatible: MIT < GPL-2.0-only", getLicenseNode("MIT", false), getLicenseNode("GPL-2.0-only", false), false}, {"incompatible: OSL-1.0 < OPL-1.0", getLicenseNode("OSL-1.0", false), getLicenseNode("OPL-1.0", false), false}, {"not simple license: (MIT OR ISC) < GPL-3.0", getLicenseNode("(MIT OR ISC)", false), getLicenseNode("GPL-3.0", false), false}, // TODO: should it raise error? } diff --git a/spdxexp/satisfies_test.go b/spdxexp/satisfies_test.go index a676d24..cc32199 100644 --- a/spdxexp/satisfies_test.go +++ b/spdxexp/satisfies_test.go @@ -18,6 +18,8 @@ func TestValidateLicenses(t *testing.T) { {"All invalid", []string{"MTI", "Apche-2.0", "0xDEADBEEF", ""}, false, []string{"MTI", "Apche-2.0", "0xDEADBEEF", ""}}, {"All valid", []string{"MIT", "Apache-2.0", "GPL-2.0"}, true, []string{}}, {"Some invalid", []string{"MTI", "Apche-2.0", "GPL-2.0"}, false, []string{"MTI", "Apche-2.0"}}, + {"GPL-2.0", []string{"GPL-2.0"}, true, []string{}}, + {"GPL-2.0-only", []string{"GPL-2.0-only"}, true, []string{}}, } for _, test := range tests { @@ -110,6 +112,8 @@ func TestSatisfies(t *testing.T) { {"GPL-1.0+ satisfies [GPL-2.0+]", "GPL-1.0+", []string{"GPL-2.0+"}, true, nil}, {"! GPL-1.0 satisfies [GPL-2.0+]", "GPL-1.0", []string{"GPL-2.0+"}, false, nil}, {"GPL-2.0-only satisfies [GPL-2.0-only]", "GPL-2.0-only", []string{"GPL-2.0-only"}, true, nil}, + {"GPL-2.0 satisfies [GPL-2.0-only]", "GPL-2.0", []string{"GPL-2.0-only"}, true, nil}, + {"GPL-2.0 AND GPL-2.0-only satisfies [GPL-2.0-only]", "GPL-2.0 AND GPL-2.0-only", []string{"GPL-2.0-only"}, true, nil}, {"GPL-3.0-only satisfies [GPL-2.0+]", "GPL-3.0-only", []string{"GPL-2.0+"}, true, nil}, {"! GPL-2.0 satisfies [GPL-2.0+ WITH Bison-exception-2.2]", From e08d1d684af267dc6d1010f86eee99cd664fec3a Mon Sep 17 00:00:00 2001 From: "E. Lynette Rayle" Date: Mon, 20 Mar 2023 11:19:49 -0400 Subject: [PATCH 2/2] fix bug where GPL-2.0 failed to match GPL-2.0-only add/refine function documentation --- spdxexp/compare.go | 16 ++++++++++++ spdxexp/license.go | 7 +++++ spdxexp/node.go | 64 +++++++++++++++++++++++++++++----------------- 3 files changed, 63 insertions(+), 24 deletions(-) diff --git a/spdxexp/compare.go b/spdxexp/compare.go index 3c77a53..72211e8 100644 --- a/spdxexp/compare.go +++ b/spdxexp/compare.go @@ -1,5 +1,13 @@ package spdxexp +// The compare methods determine if two ranges are greater than, less than or equal within the same license group. +// NOTE: Ranges are organized into groups (referred to as license groups) of the same base license (e.g. GPL). +// Groups have sub-groups of license versions (referred to as the range) where each member is considered +// to be the same version (e.g. {GPL-2.0, GPL-2.0-only}). The sub-groups are in ascending order within +// the license group, such that the first sub-group is considered to be less than the second sub-group, +// and so on. (e.g. {{GPL-1.0}, {GPL-2.0, GPL-2.0-only}} implies {GPL-1.0} < {GPL-2.0, GPL-2.0-only}). + +// compareGT returns true if the first range is greater than the second range within the same license group; otherwise, false. func compareGT(first *node, second *node) bool { if !first.isLicense() || !second.isLicense() { return false @@ -13,6 +21,7 @@ func compareGT(first *node, second *node) bool { return firstRange.location[versionGroup] > secondRange.location[versionGroup] } +// compareLT returns true if the first range is less than the second range within the same license group; otherwise, false. func compareLT(first *node, second *node) bool { if !first.isLicense() || !second.isLicense() { return false @@ -26,10 +35,15 @@ func compareLT(first *node, second *node) bool { return firstRange.location[versionGroup] < secondRange.location[versionGroup] } +// compareEQ returns true if the first and second range are the same range within the same license group; otherwise, false. func compareEQ(first *node, second *node) bool { if !first.isLicense() || !second.isLicense() { return false } + if first.lic.license == second.lic.license { + return true + } + firstRange := getLicenseRange(*first.license()) secondRange := getLicenseRange(*second.license()) @@ -39,6 +53,8 @@ func compareEQ(first *node, second *node) bool { return firstRange.location[versionGroup] == secondRange.location[versionGroup] } +// sameLicenseGroup returns false if either license isn't in a range or the two ranges are +// not in the same license group (e.g. group GPL != group Apache); otherwise, true func sameLicenseGroup(firstRange *licenseRange, secondRange *licenseRange) bool { if firstRange == nil || secondRange == nil || firstRange.location[licenseGroup] != secondRange.location[licenseGroup] { return false diff --git a/spdxexp/license.go b/spdxexp/license.go index 53b5b83..1c208a6 100644 --- a/spdxexp/license.go +++ b/spdxexp/license.go @@ -640,6 +640,13 @@ func getExceptions() []string { } } +// licenseRanges returns a list of license ranges. +// +// Ranges are organized into groups (referred to as license groups) of the same base license (e.g. GPL). +// Groups have sub-groups of license versions (referred to as the range) where each member is considered +// to be the same version (e.g. {GPL-2.0, GPL-2.0-only}). The sub-groups are in ascending order within +// the license group, such that the first sub-group is considered to be less than the second sub-group, +// and so on. (e.g. {{GPL-1.0}, {GPL-2.0, GPL-2.0-only}} implies {GPL-1.0} < {GPL-2.0, GPL-2.0-only}). func licenseRanges() [][][]string { return [][][]string{ { diff --git a/spdxexp/node.go b/spdxexp/node.go index 392e95a..7992ee0 100644 --- a/spdxexp/node.go +++ b/spdxexp/node.go @@ -89,7 +89,7 @@ func (n *node) isLicense() bool { return n.role == licenseNode } -// Return the value of the license field. +// license returns the value of the license field. // See also reconstructedLicenseString() func (n *node) license() *string { if !n.isLicense() { @@ -167,7 +167,7 @@ func (n *node) reconstructedLicenseString() *string { return nil } -// Sort an array of license and license reference nodes alphabetically based +// sortLicenses sorts an array of license and license reference nodes alphabetically based // on their reconstructedLicenseString() representation. The sort function does not expect // expression nodes, but if one is in the nodes list, it will sort to the end. func sortLicenses(nodes []*node) { @@ -186,29 +186,51 @@ func sortLicenses(nodes []*node) { // ---------------------- Comparator Methods ---------------------- -// Return true if two licenses are compatible; otherwise, false. +// licensesAreCompatible returns true if two licenses are compatible; otherwise, false. +// Two licenses are compatible if they are the same license or if they are in the same +// license group and they meet one of the following rules: +// +// * both licenses have the `hasPlus` flag set to true +// * the first license has the `hasPlus` flag and the second license is in the first license's range or greater +// * the second license has the `hasPlus` flag and the first license is in the second license's range or greater +// * both licenses are in the same range func (nodes *nodePair) licensesAreCompatible() bool { + // checking ranges is expensive, so check for simple cases first if !nodes.firstNode.isLicense() || !nodes.secondNode.isLicense() { return false } + if !nodes.exceptionsAreCompatible() { + return false + } + if nodes.licensesExactlyEqual() { + return true + } + + // simple cases don't apply, so check license ranges + // NOTE: Ranges are organized into groups (referred to as license groups) of the same base license (e.g. GPL). + // Groups have sub-groups of license versions (referred to as the range) where each member is considered + // to be the same version (e.g. {GPL-2.0, GPL-2.0-only}). The sub-groups are in ascending order within + // the license group, such that the first sub-group is considered to be less than the second sub-group, + // and so on. (e.g. {{GPL-1.0}, {GPL-2.0, GPL-2.0-only}} implies {GPL-1.0} < {GPL-2.0, GPL-2.0-only}). if nodes.secondNode.hasPlus() { if nodes.firstNode.hasPlus() { - // first+, second+ + // first+, second+ just need to be in same range group return nodes.rangesAreCompatible() } - // first, second+ + // first, second+ requires first to be in range of second return nodes.identifierInRange() } // else secondNode does not have plus if nodes.firstNode.hasPlus() { - // first+, second + // first+, second requires second to be in range of first revNodes := &nodePair{firstNode: nodes.secondNode, secondNode: nodes.firstNode} return revNodes.identifierInRange() } - // first, second - return nodes.licensesExactlyEqual() + // first, second requires both to be in same range group + return nodes.rangesEqual() } +// licenseRefsAreCompatible returns true if two license references are compatible; otherwise, false. func (nodes *nodePair) licenseRefsAreCompatible() bool { if !nodes.firstNode.isLicenseRef() || !nodes.secondNode.isLicenseRef() { return false @@ -222,13 +244,9 @@ func (nodes *nodePair) licenseRefsAreCompatible() bool { return compatible } -// Return true if two licenses are compatible in the context of their ranges; otherwise, false. +// licenseRefsAreCompatible returns true if two licenses are in the same license group (e.g. all "GPL" licenses are in the same +// license group); otherwise, false. func (nodes *nodePair) rangesAreCompatible() bool { - if nodes.licensesExactlyEqual() { - // licenses specify ranges exactly the same (e.g. Apache-1.0+, Apache-1.0+) - return true - } - firstNode := *nodes.firstNode secondNode := *nodes.secondNode @@ -238,7 +256,7 @@ func (nodes *nodePair) rangesAreCompatible() bool { // When both licenses allow later versions (i.e. hasPlus==true), being in the same license // group is sufficient for compatibility, as long as, any exception is also compatible // Example: All Apache licenses (e.g. Apache-1.0, Apache-2.0) are in the same license group - return sameLicenseGroup(firstRange, secondRange) && nodes.exceptionsAreCompatible() + return sameLicenseGroup(firstRange, secondRange) } // identifierInRange returns true if the (first) simple license is in range of the (second) @@ -247,14 +265,7 @@ func (nodes *nodePair) identifierInRange() bool { simpleLicense := nodes.firstNode plusLicense := nodes.secondNode - if !compareGT(simpleLicense, plusLicense) && !compareEQ(simpleLicense, plusLicense) { - return false - } - - // With simpleLicense >= plusLicense, licenses are compatible, as long as, any exception - // is also compatible - return nodes.exceptionsAreCompatible() - + return compareGT(simpleLicense, plusLicense) || compareEQ(simpleLicense, plusLicense) } // exceptionsAreCompatible returns true if neither license has an exception or they have @@ -274,10 +285,15 @@ func (nodes *nodePair) exceptionsAreCompatible() bool { } return *nodes.firstNode.exception() == *nodes.secondNode.exception() +} +// rangesEqual returns true if the licenses are in the same range; otherwise, false +// (e.g. GPL-2.0-only == GPL-2.0) +func (nodes *nodePair) rangesEqual() bool { + return compareEQ(nodes.firstNode, nodes.secondNode) } -// Return true if the licenses are the same; otherwise, false +// licensesExactlyEqual returns true if the licenses are the same; otherwise, false func (nodes *nodePair) licensesExactlyEqual() bool { return strings.EqualFold(*nodes.firstNode.reconstructedLicenseString(), *nodes.secondNode.reconstructedLicenseString()) }