Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[http] extract Multipart code into new ServletRequestMultipartWrapper #367

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stbischof
Copy link
Contributor

Mostly in the usage of felix-jetty-light there is the cases where you do not need the multipart support and can remove the dependencies of common-fileupload and common-io. To be able to do this we could not load a instantiate a class that imports common-fileupload classes. So we have to free the old class ServletRequestWrapper of this imports and add a ServletRequestMultipartWrapper that still have the multipart support.

Mostly in the usage of felix-jetty-light there is the cases where you do
not need the multipart support and can remove the dependencies of
common-fileupload and common-io. To be able to do this we could not load
a instantiate a class that imports common-fileupload classes. So we have
to free the old class ServletRequestWrapper of this imports and add a
ServletRequestMultipartWrapper that still have the multipart support.

Signed-off-by: Stefan Bischof <[email protected]>
@paulrutter
Copy link
Contributor

I haven't checked the whole PR yet, but i would expect commons-fileupload to be marked optional in the pom.xml of the base bundle.
Otherwise the bundle wouldn't even start if those classes are not on the classpath, right?

@stbischof
Copy link
Contributor Author

stbischof commented Jan 10, 2025

yes the imports are is marked as optional. this way it could resolve and start without commons-fileupload.
And this is what i do,o i do not used the dependency on runtime.

But the class ServletRequestWrapper imports classes from commons-fileupload
https://github.com/apache/felix-dev/pull/367/files#diff-b1cfaf93f83551a9004116c63a435ac8e64b198c1e8ee68f4178d3b59073568dL51-L56

But in any case the ServletRequestWrapper must be instantiated. But this will fails because the classes are`t there

@paulrutter
Copy link
Contributor

I see, but wouldn't it make sense to mark the dependencies as optional as well, in

<dependency>
?

@laeubi
Copy link

laeubi commented Jan 10, 2025

but i would expect commons-fileupload to be marked optional in the pom.xml

Please note that "optional" means something totally different to maven, see https://maven.apache.org/guides/introduction/introduction-to-optional-and-excludes-dependencies.html

Optional is similar to "provided" but should not be included into runtime collections e.g. slf4-api will be compile and slf4-simple will be optional runtime because one might choose a different implementation of the API but it wont work without any provided!

Optional in OSGi mean it could work with or without this dependency, is really hard to get correct and almost never useful if not carefully crafted and tested. Especially in this case it might lead to hard to track runtime errors, so ClassNotFound Exceptions should be catched somewhere and output a meaningful message so the user understands how to resolve the issue.

@paulrutter
Copy link
Contributor

paulrutter commented Jan 10, 2025

Thanks for clarifying @laeubi, that makes sense 👍

It's already optional in the jetty12 bundle, is that considered a bug then?

<optional>true</optional>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants