jbonofre commented on PR #714:
URL: https://github.com/apache/camel-karaf/pull/714#issuecomment-5421190241
## Review findings: this can be reduced to a 5-line change
I went back over this backport and verified the root cause and the packaging
behaviour by building the module. The fix is correct and CI is green, but the
Java class it adds is redundant — upstream Camel already ships it — and once
it's dropped, the two new dependencies and the `camel.osgi.export` change
become unnecessary too.
### Root cause (verified)
Upstream never registers `simple-no-file` in the factory finder.
`DefaultLanguageResolver` hardcodes it:
```java
//
core/camel-base-engine/src/main/java/org/apache/camel/impl/engine/DefaultLanguageResolver.java:47
if ("simple-no-file".equals(name)) {
type = context.getClassResolver().resolveClass(SIMPLE_NO_FILE);
}
```
I scanned every Camel 4.18.2 artifact (1024 jars): **none** ships
`META-INF/services/org/apache/camel/language/simple-no-file`, and none ships a
`language/resolver/default` entry either. So in Karaf, `OsgiLanguageResolver`
misses the OSGi service lookup, then its `getLanguageResolver("default")`
fallback finds nothing — nothing ever registers `resolver=default` — and throws
`NoSuchLanguageException`. That matches the trace in #707.
The mechanism this PR relies on is sound: `Activator.registerLanguages`
enumerates
`bundle.getEntryPaths("META-INF/services/org/apache/camel/language/")` and
registers a `BundleLanguageResolver` per entry with property `language=<name>`,
and `BaseResolver.createInstance` reads `class=` and calls
`bundle.loadClass(...)`.
### The class is already in the bundle
`camel-core-languages-4.18.2.jar` already contains
`org/apache/camel/language/simple/SimpleNoFileLanguage.class`, and this bundle
already includes and exports `org.apache.camel.language.simple`. **Only the
service file is missing.**
Keeping our own copy has three concrete downsides:
* The shaded jar ends up shipping *our* copy (585 bytes) rather than
upstream's (394 bytes) — I confirmed which one wins. That silently forks a
Camel core class and shadows any future upstream change to it.
* Our copy adds `@Language(value = "simple-no-file", functionsClass =
SimpleConstants.class)`, which **upstream deliberately does not have**. That
omission is precisely why no service file is generated: the javadoc states the
language is internal-only and not a public standard language.
* It calls the **package-private** `SimpleLanguage(boolean
skipFileFunctions)` constructor from a different Maven module. It compiles
today, but breaks in a confusing way if upstream ever changes that signature or
visibility.
### Suggested minimal patch (built and verified)
New file
`core/camel-core-languages/src/main/resources/META-INF/services/org/apache/camel/language/simple-no-file`:
```properties
class=org.apache.camel.language.simple.SimpleNoFileLanguage
```
And in `core/camel-core-languages/pom.xml`, add a second entry next to the
existing one:
```xml
<resources>
<resource>
<directory>../../src/main/resources</directory>
<filtering>false</filtering>
</resource>
<resource>
<directory>src/main/resources</directory>
<filtering>false</filtering>
</resource>
</resources>
```
That second `<resource>` is needed because the existing `<resources>` block
overrides the Maven default and lists only `../../src/main/resources`, so this
module's own `src/main/resources` is never copied into `target/classes`. That
is the real reason the shade `IncludeResourceTransformer` was required. Wiring
the directory in fixes it generally, instead of one file at a time — otherwise
the next resource added to this module gets silently dropped again.
Then `SimpleNoFileLanguage.java`, both new dependencies, the
`<transformers>` block and the `camel.osgi.export` change can all be dropped.
Net diff:
```
core/camel-core-languages/pom.xml | 4 ++++
.../META-INF/services/org/apache/camel/language/simple-no-file | 1 +
2 files changed, 5 insertions(+)
```
I built this variant: the service file is in the bundle,
`SimpleNoFileLanguage.class` is upstream's 394-byte one, `Export-Package` is
byte-identical to the base branch, `LICENSE.txt`/`NOTICE.txt` are retained, and
the bundle is 412,019 bytes vs 412,261 for this PR.
### On the `camel.osgi.export` narrowing
For the record, that change was **not** gratuitous — it is required given
the two new dependencies. I tested keeping `org.apache.camel*` alongside them,
and bnd pulls all of camel-api and camel-support into the bundle: **412 KB →
1.9 MB**, with 44 extra exported packages including `org.apache.camel.spi`,
`org.apache.camel.support` and `org.apache.camel.util`, which would duplicate
the exports of our dedicated `camel-api` and `camel-support` bundles. Good
catch.
Two caveats if the dependencies are kept anyway:
* They are at default **compile** scope, so they leak into this artifact's
transitive graph as non-OSGi upstream jars. Every other dependency in this
module excludes all of `org.apache.camel`; these should at least be `provided`.
* Narrowing to `org.apache.camel.language*` is a silent guard: if a future
Camel release adds a non-`language` package to `camel-core-languages`, it will
stop being exported with no build failure. Today it is a verified no-op.
### Two other points
* **No test.** `tests/features/camel-core/` is the natural home (it already
has `CamelFileRouteSupplier` and `CamelCoreITest`). Asserting
`context.resolveLanguage("simple-no-file")` resolves, or a `pollEnrich` route
with a dynamic file expression, would keep a future Camel upgrade from
regressing this silently.
* **Follow-up worth filing:** this fixes one name, but
`OsgiLanguageResolver` still has no working fallback to
`DefaultLanguageResolver`, so any future special case added there will break
under Karaf in exactly the same way. Either mirror the special case or register
a `resolver=default` `LanguageResolver`.
### Process note
#708, the original PR against `main`, is still open. Merging this one first
puts the fix on the maintenance branch while `main` lacks it, and if #708
changes during review the two branches carry different fixes. Landing #708 on
`main` first and cherry-picking the merged commit would keep them in sync.
### Security model
Not a widening under `docs/modules/ROOT/pages/security-model.adoc`:
flat-classpath Camel resolves `simple-no-file` too, via the explicit special
case above, so this restores parity rather than opening a sink Camel core had
closed. The language name comes from `PollReifier` in the route definition,
never from message data.
---
_Claude Code on behalf of JB Onofré_
--
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]
