Replace OTHER with a LicenseRef#926
Replace OTHER with a LicenseRef#926dangoor merged 4 commits intoactions:mainfrom dangoor:dangoor/replace-other
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that the invalid OTHER token in license expressions is replaced with a valid LicenseRef-clearlydefined-OTHER before parsing or satisfiability checks.
- Sanitizes input expressions in
satisfies,satisfiesAny,satisfiesAll, andisValidby callingremoveInvalidSPDX. - Introduces
removeInvalidSPDX(with a regex) to transformOTHERintoLicenseRef-clearlydefined-OTHER. - Expands unit tests to cover the new replacement behavior.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/spdx.ts | Preprocess SPDX strings by replacing OTHER with LicenseRef-clearlydefined-OTHER. |
| tests/spdx.test.ts | Adds test cases for OTHER in various positions and a new removeInvalidSPDX suite. |
Comments suppressed due to low confidence (1)
tests/spdx.test.ts:290
- Add a test case for multiple occurrences of
OTHER(e.g.,'OTHER AND OTHER') to verify that all instances are correctly replaced.
const units = [
src/spdx.ts
Outdated
| candidateExpr: string, | ||
| constraintExpr: string | ||
| ): boolean { | ||
| candidateExpr = removeInvalidSPDX(candidateExpr) |
There was a problem hiding this comment.
Constraint expressions are not sanitized in satisfies. Consider also calling removeInvalidSPDX on constraintExpr to ensure OTHER in constraints is handled.
| candidateExpr = removeInvalidSPDX(candidateExpr) | |
| candidateExpr = removeInvalidSPDX(candidateExpr) | |
| constraintExpr = removeInvalidSPDX(constraintExpr) |
There was a problem hiding this comment.
This is probably unnecessary since the constraint expressions are written by the users and OTHER is invalid already. Users should write LicenseRef-clearlydefined-OTHER. That does bring to mind an issue with the approach in this PR: when an invalid license message is shown, it would still say OTHER instead of LicenseRef-clearlydefined-OTHER, so it wouldn't give users a clue about what they'd need to add to their license list. (Generally speaking, they shouldn't be adding OTHER in any form to their allowed license list because they can't know what the terms of that license are!)
There was a problem hiding this comment.
I spoke with @ahpook about this particular UX nit and he wasn't worried about it in the context of all of the good this change does in general. (Plus, from a compliance perspective, people really should not be adding LicenseRef-clearlydefined-OTHER to their allow lists because you can't be sure what the terms you're agreeing to are.)
Ahmed3lmallah
left a comment
Ahmed3lmallah
left a comment
There was a problem hiding this comment.
LGTM besides minor comments. The dist needs to be regenerated too.
__tests__/spdx.test.ts
Outdated
| licenses: ['MIT'], | ||
| expected: true | ||
| }, | ||
| // TODO(dangoor): this does not seem correct to me: the only license is Apache-2.0 and it's on the list |
There was a problem hiding this comment.
Given that this tests satisfiesAll, I'm assuming that the candidate should satisfy all entries in the licenses list, which seems to be correctly not the case here.
There was a problem hiding this comment.
Okay, I understand. I don't see why someone would want that, but 🤷 . I'm tempted to just remove this function, since we don't use it.
| @@ -130,6 +141,16 @@ describe('satisfiesAll', () => { | |||
| candidate: 'MIT OR ISC', | |||
| licenses: ['MiT'], | |||
There was a problem hiding this comment.
Not related to this PR, but here is another case where I would have expected the result to be true but it's likely not because it's case sensitive. Does the matching here need to be case sensitive?
There was a problem hiding this comment.
Hmm, so the official spec says that license identifiers are case-insensitive.
This, then, is not actually the correct behavior as per the spec, but is the behavior expected of the library we're using. Definitely out of scope for this PR.
src/spdx.ts
Outdated
|
|
||
| // adjusts license expressions to not include the invalid `OTHER` | ||
| // which ClearlyDefined adds to license strings | ||
| export function removeInvalidSPDX(spdxExpr: string): string { |
There was a problem hiding this comment.
The method name initially threw me off. I thought we are removing an entry from a list, but after looking at the data types I realized I was wrong. Perhaps something like cleanInvalidSPDX would be more clear?
|
I'm going to need to revise this a little based on #927. |
ClearlyDefined uses the string `OTHER` for the declared license when a human has reviewed `NOASSERTION` text and found it to be a valid license, but one without an SPDX identifier. `OTHER`, unlike `NOASSERTION`, is not valid. With this change, when `OTHER` appears in a license string, we'll replace it with `LicenseRef-clearlydefined-OTHER`, which _is_ valid and will allow the expressions to parse.
|
Oof, I didn't realize I had been working from a fork and not a branch. I pushed a change that is rebased on #927. |
|
This is ready for review again, now that #927 has landed. |
3 participants
Purpose
ClearlyDefined uses the string
OTHERfor the declared license whena human has reviewed
NOASSERTIONtext and found it to be a validlicense, but one without an SPDX identifier.
OTHER, unlikeNOASSERTION, is not valid. With this change, whenOTHERappearsin a license string, we'll replace it with
LicenseRef-clearlydefined-OTHER, which is valid and will allowthe expressions to parse.
Related Issues
N/A