paulrutter commented on PR #587:
URL: https://github.com/apache/felix-dev/pull/587#issuecomment-6561686811
## Review summary
**What it does:** Typed attributes (`name:Long=42`, `List<Version>`, etc.)
are now converted for `Export-Package`, `Import-Package`,
`DynamicImport-Package`, `Require-Bundle` and `Fragment-Host`, not only the
capability headers. List values in requirements become one filter per element.
The `version`/`specification-version` check now compares versions instead of
strings.
**Verdict:** The approach is sound and small, and it reuses the existing
conversion code. The tests check both the parsed values and that the generated
requirement matches the typed capability. CI is green on JDK 17/21/25/27. I
reviewed by reading the code and did not run the build locally. No blocking
issues, only the points below.
### Points to consider
1. **Error handling** (`ManifestParser.convertTypedAttributes`)
A malformed value such as `Long=abc` throws a raw
`NumberFormatException`/`IllegalArgumentException` with no attribute name. This
is now reachable from five more headers. Consider rethrowing as
`BundleException` (`MANIFEST_ERROR`) with the attribute key and type.
2. **Running twice** (`ManifestParser.convertTypedAttributes`)
`m_types` is not cleared after conversion, so a second run on the same
clause would corrupt list values (`"[a, b]"` gets re-split). Nothing does this
today, but clearing the entry or adding a comment would protect against future
changes.
3. **Empty list** (`SimpleFilter.convert`)
`colors:List<String>=""` adds no filter, so the requirement silently
matches anything. Please reject it, warn, or at least test and document the
behavior.
4. **List semantics** (`SimpleFilter.convert`)
"A list attribute matches if the capability contains **all** values" is a
design choice. I could not find it defined for `Import-Package` in the spec.
Please document it in the Javadoc, which still says equality is assumed for
everything except version ranges. Please also confirm it matches what the TCK
in osgi/org.osgi.annotation.bundle#24 expects.
5. **Good catch, needs a test** (`SimpleFilter.convert`)
Changing `attrs.size() > 1` to `filters.size() > 1` fixes an existing
bug: a single bounded `VersionRange` produced two filters but `attrs.size() ==
1`, so the filter ended up `null`. Please add a `SimpleFilterTest` case for it
(single bounded `VersionRange`, and an empty list next to another attribute).
6. **Version-equality check** (import, dynamic import, export)
- The same block now exists three times; a small helper would remove the
duplication.
- Accepting `1.0` vs `1.0.0` is a behavior change worth a line in the
commit message.
- Error messages are less clear for invalid input, e.g. a `List`-typed
`version` shows up as `"[1.0.0, 2.0.0]"` in a parse error.
7. **Test gaps**
- All new tests are positive. Please add a negative case (a non-matching
list element or typed value must **not** match) and a malformed-value case.
- `Require-Bundle` and `Fragment-Host` have no test with a typed
`bundle-version`.
- Nit: move the `TYPED_*` constants and helpers to the bottom of the test
class.
---
_Generated by [Claude Code](https://claude.ai/code)_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
