Identifier type aliases #9900
Identifier type aliases #9900
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@oss-review-toolkit/core-devs This is an attempt to solve the issue we currently have with the renaming of I would like to get your feedback if you would prefer this approach or reverting the change, or if you have a better idea. But for the future we need to find a way to make such changes backward compatible. I'm out of office tomorrow, but we need to find a solution for this by Monday, so early feedback is appreciated. |
model/src/main/kotlin/Identifier.kt
Outdated
| * Return whether the [type][Identifier.type] of this [Identifier] matches the type of [other] by comparing them | ||
| * case-insensitively. | ||
| */ | ||
| fun Identifier.typeMatches(other: Identifier) = type.equals(other.type, ignoreCase = true) |
There was a problem hiding this comment.
How about naming this typeEquals() to align with the underlying implementation (and imo better fits)?
| id.typeMatches(Identifier.EMPTY.copy(type = "TYPE")) shouldBe true | ||
| } | ||
|
|
||
| "return false if the type does not match" { |
There was a problem hiding this comment.
Better "is not equal ignoring case" instead of "not match", for symmetry.
Make the matching of the identifier type in package configurations case insensitive to align with package curations. Signed-off-by: Martin Nonnenmacher <[email protected]>
This prepares for extending the logic in a following commit. Signed-off-by: Martin Nonnenmacher <[email protected]>
Add a map to define aliases for renamed identifier types and use that when matching types. Ideally, the aliases should come from the package manager implementations where the renaming takes place, but that is not possible because the matching logic is inside the `model` module which is a dependency of the package manager plugins. Signed-off-by: Martin Nonnenmacher <[email protected]>
mnonnenmacher
force-pushed
the
identifier-type-aliases
branch
from
6903b70 to
aa94156
Compare
|
@fviernau Before I address the comments, would you agree with the general approach? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9900 +/- ##
============================================
- Coverage 68.43% 68.41% -0.02%
Complexity 1308 1308
============================================
Files 250 250
Lines 8880 8884 +4
Branches 921 924 +3
============================================
+ Hits 6077 6078 +1
Misses 2413 2413
- Partials 390 393 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
double check: The change only switch from equals to equalsIgnoreCase right ? |
Not sure if I'm getting the question right, but this change is not only a change to ignore the case, it also introduces a mapping for backwards compatibility. private val typeAliases = mapOf(
"spdxdocument" to "spdxdocumentfile"
)Would you agree with this approach, @fviernau? |
Indeed, I missed the third commit. (Which is the most important one I guess)
Hm, so can you explain why it is not possible to change the type in your customers curations? I guess, the problem is that the curations are not in a cenral repository, but in
What do you think? |
ORT offers both, curations in a central repository and curations in
Yes, the individual user can fix the issue quickly.
Yes, that would all work, but as said, we don't think that it was worth breaking the users Do I read your message correctly, that you are against both laid out proposals by @mnonnenmacher?
|
I'm not against them. I just wanted to figure out whether there is an alternative. My preference would be to revert the change, but if you prefer to merge this PR as a solution edit: Would a revert actually still guarantee that |
|
one more idea to share: alternatively the de-serialization could be adjusted to do the mapping. |
| /** | ||
| * A map of lowercase aliases for types that were renamed. | ||
| */ | ||
| private val typeAliases = mapOf( |
There was a problem hiding this comment.
I'm not sure whether a map is the right data structure here. To me, aliases are a set of alternative names, not a one-to-one relationship. I'd propose more something like we have for VcsType aliases, i.e. a list of names that are all treated equally (when consuming), and by convention use the first entry in the list when producing.
There was a problem hiding this comment.
that is not possible because the matching logic is inside the
modelmodule which is a dependency of the package manager plugins.
Well, the matching logic could stay in the model, but the definition of aliases could still be moved to the plugins, simply by making projectType a list of aliases instead of a single string, as described above.
There was a problem hiding this comment.
How should the logic in model get access to the aliases if they are defined in Gradle modules that depend on model? It doesn't even know what projectTypes the plugins define.
There was a problem hiding this comment.
How should the logic in
modelget access to the aliases if they are defined in Gradle modules that depend onmodel?
Well, basically by creating instances via all factories for the package manager entry point. That super-dump, but technically possible, I believe.
It doesn't even know what
projectTypes the plugins define.
Yes, that the projectTypes are not known to the factories already has caused me trouble with #9780 as well...
There was a problem hiding this comment.
Well, basically by creating instances via all factories for the package manager entry point. That super-dump, but technically possible, I believe.
The model doesn't know about package manager factories because the interface is part of the analyzer module. I think a proper implementation requires to move the matching logic to a separate module which can depend on the analyzer for that reason.
It could be that a bug slipped in when introducing the should probably not unconditionally use Also, I'd like to better understand how exactly your users are affected. Does it mostly break package curations, or package configurations? And for the latter, configurations for packages or for projects? I'm not 100% convinced yet that we need such an alias mechanism going forward, but it could become useful again when adding a "Project" suffix to the ID types of projects. That's a use-case to keep in mind. |
Nice finding. Could this be the root cause of the observed issue? |
I'm not very happy about this implementation either, so I have created #9910 to revert the change instead.
No, but that is neither the case now nor was it the case before. |
It's both, package configurations and package curations defined in .ort.yml files. We have more than 50 affected repositories so the effort to update all of them is huge, especially if the reason for the change is mainly cosmetic right now. When I reviewed the PR that introduced the change I completely underestimated the consequences.
I believe we need such a mechanism for user facing configuration changes to allow for smooth migrations. If we add the "Project" suffix we cannot expect our users to update hundreds of repositories from one day to the other without having a way to at least give them some time for the migration. |
|
Closed in favor of #9910. |
Add a mechanism to define aliases for renamed identifier types to ensure package curations and package configurations can still be matched after a renaming. Please see the commit messages for details.