Define precedence between expressions and identifiers on the read path

Summary

PackageLicenses#spdx_id_for caches id => spdx_identifier only. Expression rows have no spdx_identifier (one row = identifier XOR expression per #606521 (closed)), so lookups return nil. Report#licenses sorts on license.name.downcase, which raises on nil. Expression rows are effectively invisible to policy evaluation.

Proposal

Prefer spdx_expression over spdx_identifier when building the report that feeds LicenseExpressionChecker. When a package has both an identifier row and an expression row in its tuple (coexistence), emit only the expression entry; emitting both causes bare-identifier rows to fire denylist policies incorrectly (see Decisions).

Expression rows must pass through license_name_for raw. Converting them to catalogue names (e.g. "MIT License") breaks SPDX AST parsing downstream.

Implementation

  • spdx_id_for: change cache value to spdx_expression.presence || spdx_identifier
  • license_name_for: add || spdx_id fallback so expressions bypass the catalogue lookup and reach the parser as raw strings
  • url_for: return nil when spdx_id contains a space; expressions are never valid SPDX registry slugs and bare identifiers never contain spaces
  • licenses_with_names_for: after resolving all IDs for a package, if any entry came from an expression row, drop the identifier-row entries from the same set. Store {value:, is_expression:} in the cache rather than re-detecting from the string.

The CycloneDX path (licenses_for_component) already handles this correctly. Follow the same pattern.

Decisions

Expression wins over identifier. Agreed by @onaaman, @g.hickman, @cwidstrom, and @mc_rocha in #606525 (closed) thread. The first iteration uses field presence, not content: if spdx_expression is set, it wins regardless of whether it subsumes the identifier.

Conflict detection is deferred. Cases where identifier and expression contradict (e.g. MIT identifier vs LGPL-2.1+ expression) are a follow-up tracked under the conflict resolution epic. This MR does not flag or resolve conflicts.

Emitting both is not neutral. With a denylist of {MIT}, a sibling bare MIT entry causes a violation even though MIT OR Apache-2.0 correctly would not. violates_or_node? requires every alternative to be denied; a separate bare MIT row bypasses that gate. Expression must replace, not coexist with, identifier for the policy report.

Read-path suppression, not write-path. Both rows are stored in pm_licenses and referenced in the tuple. Filtering at the write path would orphan identifier rows and lose data. Business logic for precedence belongs in the read path.

url_for returns nil for expressions. No spdx.org page exists for compound expressions. Callers already handle nil URL.

Test plan

ee/spec/lib/gitlab/license_scanning/package_licenses_spec.rb:

  • spdx_id_for returns expression string for an expression row
  • spdx_id_for returns identifier string for an identifier row (regression)
  • license_name_for with an expression string returns the expression unchanged
  • url_for("MIT OR Apache-2.0") returns nil
  • url_for("MIT") returns the spdx.org URL (regression)
  • licenses_with_names_for with a coexistence ID set returns one entry containing the expression, no identifier entry
  • licenses_with_names_for with identifier-only IDs returns unchanged behavior (regression)
  • Report#licenses does not raise when the set includes an expression row

Dependencies

#606521 (closed) (adds spdx_expression column), !250673 (merged) (MR1), !250674 (merged) (MR2)

Key files

  • ee/lib/gitlab/license_scanning/package_licenses.rb
  • ee/spec/lib/gitlab/license_scanning/package_licenses_spec.rb
Edited by 🤖 GitLab Bot 🤖