Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
paulrutter commented on PR #367: URL: https://github.com/apache/felix-dev/pull/367#issuecomment-2587223035 @laeubi See https://github.com/apache/felix-dev/pull/368 -- 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: dev-unsubscr...@felix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
paulrutter commented on PR #367: URL: https://github.com/apache/felix-dev/pull/367#issuecomment-2587211098 I will make that change to the jetty12 bundle then. -- 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: dev-unsubscr...@felix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
laeubi commented on PR #367: URL: https://github.com/apache/felix-dev/pull/367#issuecomment-2587205869 @paulrutter I think "optional" is sued semantically wrong here from maven point of view, but as the artifact is most likely never used as a maven dependency (but only as OSGi artifact it might not really matter, I would just remove all `` and let them be simple compile scope. -- 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: dev-unsubscr...@felix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
stbischof merged PR #367: URL: https://github.com/apache/felix-dev/pull/367 -- 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: dev-unsubscr...@felix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
paulrutter commented on PR #367: URL: https://github.com/apache/felix-dev/pull/367#issuecomment-2587161604 @stbischof Yes 👍🏻 The only thing i was wondering now is if the optional stated here (jetty12 bundle only) https://github.com/apache/felix-dev/blob/f572bd8131df53e1fd39e9655afe6c7758541cc1/http/jetty12/pom.xml#L738 is then correct. From what i understood from @laeubi, that `optional` should not be there, right? -- 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: dev-unsubscr...@felix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
stbischof commented on PR #367: URL: https://github.com/apache/felix-dev/pull/367#issuecomment-2587145210 also tested in my application. Now works proper without `file-upload` and `commons.io` @paulrutter okay with a merge? -- 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: dev-unsubscr...@felix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
cziegeler commented on PR #367: URL: https://github.com/apache/felix-dev/pull/367#issuecomment-2587094116 Thanks, looks good -- 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: dev-unsubscr...@felix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
stbischof commented on PR #367: URL: https://github.com/apache/felix-dev/pull/367#issuecomment-2586948785 @cziegeler I now throw an ServletException on the 2 Messages. see my last commit. -- 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: dev-unsubscr...@felix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
stbischof commented on code in PR #367:
URL: https://github.com/apache/felix-dev/pull/367#discussion_r1913096524
##
http/base/src/main/java/org/apache/felix/http/base/internal/dispatch/ServletRequestWrapper.java:
##
@@ -411,143 +381,17 @@ public boolean isAsyncSupported()
return this.asyncSupported;
}
-private RequestContext getMultipartContext() {
-final RequestContext multipartContext;
-if (!POST_METHOD.equalsIgnoreCase(this.getMethod())) {
-multipartContext = null;
-} else {
-multipartContext = new RequestContext() {
-
-@Override
-public InputStream getInputStream() throws IOException {
-return ServletRequestWrapper.this.getInputStream();
-}
-
-@Override
-public String getContentType() {
-return ServletRequestWrapper.this.getContentType();
-}
-
-@Override
-public int getContentLength() {
-return ServletRequestWrapper.this.getContentLength();
-}
-
-@Override
-public String getCharacterEncoding() {
-return ServletRequestWrapper.this.getCharacterEncoding();
-}
-};
-}
-return multipartContext;
-}
-
-private Collection checkMultipart() throws IOException,
ServletException {
-if ( parts == null ) {
-final RequestContext multipartContext = getMultipartContext();
-if ( multipartContext != null &&
FileUploadBase.isMultipartContent(multipartContext) ) {
-if ( this.multipartConfig == null) {
-throw new IllegalStateException("Multipart not enabled for
servlet.");
-}
-
-if ( System.getSecurityManager() == null ) {
-handleMultipart(multipartContext);
-} else {
-final AccessControlContext ctx =
bundleForSecurityCheck.adapt(AccessControlContext.class);
-final IOException ioe = AccessController.doPrivileged(new
PrivilegedAction() {
-
-@Override
-public IOException run() {
-try {
-handleMultipart(multipartContext);
-} catch ( final IOException ioe) {
-return ioe;
-}
-return null;
-}
-}, ctx);
-if ( ioe != null ) {
-throw ioe;
-}
-}
-
-} else {
-throw new ServletException("Not a multipart request");
-}
-}
-return parts;
-}
-
-private void handleMultipart(final RequestContext multipartContext) throws
IOException {
-// Create a new file upload handler
-final FileUpload upload = new FileUpload();
-upload.setSizeMax(this.multipartConfig.multipartMaxRequestSize);
-upload.setFileSizeMax(this.multipartConfig.multipartMaxFileSize);
-upload.setFileItemFactory(new
DiskFileItemFactory(this.multipartConfig.multipartThreshold,
-new File(this.multipartConfig.multipartLocation)));
-upload.setFileCountMax(this.multipartConfig.multipartMaxFileCount);
-// Parse the request
-List items = null;
-try {
-items = upload.parseRequest(multipartContext);
-} catch (final FileUploadException fue) {
-throw new IOException("Error parsing multipart request", fue);
-}
-this.parts = new ArrayList<>();
-for(final FileItem item : items) {
-this.parts.add(new PartImpl(item));
-}
-}
-
@Override
@SuppressWarnings({ "unchecked", "rawtypes" })
public Collection getParts() throws IOException, ServletException {
-return (Collection)checkMultipart();
-
+return null;
Review Comment:
Thank you!
I now throw an ServletException.
##
http/base/src/main/java/org/apache/felix/http/base/internal/dispatch/ServletRequestWrapper.java:
##
@@ -411,143 +381,17 @@ public boolean isAsyncSupported()
return this.asyncSupported;
}
-private RequestContext getMultipartContext() {
-final RequestContext multipartContext;
-if (!POST_METHOD.equalsIgnoreCase(this.getMethod())) {
-multipartContext = null;
-} else {
-multipartContext = new RequestContext() {
-
-@Override
-public InputStream getInputStream() throws IOException {
-return ServletRequestWrapper.this.getInputStream();
-}
-
-@Override
-public String getContentType() {
-
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
cziegeler commented on code in PR #367:
URL: https://github.com/apache/felix-dev/pull/367#discussion_r1912730567
##
http/base/src/main/java/org/apache/felix/http/base/internal/dispatch/ServletRequestWrapper.java:
##
@@ -411,143 +381,17 @@ public boolean isAsyncSupported()
return this.asyncSupported;
}
-private RequestContext getMultipartContext() {
-final RequestContext multipartContext;
-if (!POST_METHOD.equalsIgnoreCase(this.getMethod())) {
-multipartContext = null;
-} else {
-multipartContext = new RequestContext() {
-
-@Override
-public InputStream getInputStream() throws IOException {
-return ServletRequestWrapper.this.getInputStream();
-}
-
-@Override
-public String getContentType() {
-return ServletRequestWrapper.this.getContentType();
-}
-
-@Override
-public int getContentLength() {
-return ServletRequestWrapper.this.getContentLength();
-}
-
-@Override
-public String getCharacterEncoding() {
-return ServletRequestWrapper.this.getCharacterEncoding();
-}
-};
-}
-return multipartContext;
-}
-
-private Collection checkMultipart() throws IOException,
ServletException {
-if ( parts == null ) {
-final RequestContext multipartContext = getMultipartContext();
-if ( multipartContext != null &&
FileUploadBase.isMultipartContent(multipartContext) ) {
-if ( this.multipartConfig == null) {
-throw new IllegalStateException("Multipart not enabled for
servlet.");
-}
-
-if ( System.getSecurityManager() == null ) {
-handleMultipart(multipartContext);
-} else {
-final AccessControlContext ctx =
bundleForSecurityCheck.adapt(AccessControlContext.class);
-final IOException ioe = AccessController.doPrivileged(new
PrivilegedAction() {
-
-@Override
-public IOException run() {
-try {
-handleMultipart(multipartContext);
-} catch ( final IOException ioe) {
-return ioe;
-}
-return null;
-}
-}, ctx);
-if ( ioe != null ) {
-throw ioe;
-}
-}
-
-} else {
-throw new ServletException("Not a multipart request");
-}
-}
-return parts;
-}
-
-private void handleMultipart(final RequestContext multipartContext) throws
IOException {
-// Create a new file upload handler
-final FileUpload upload = new FileUpload();
-upload.setSizeMax(this.multipartConfig.multipartMaxRequestSize);
-upload.setFileSizeMax(this.multipartConfig.multipartMaxFileSize);
-upload.setFileItemFactory(new
DiskFileItemFactory(this.multipartConfig.multipartThreshold,
-new File(this.multipartConfig.multipartLocation)));
-upload.setFileCountMax(this.multipartConfig.multipartMaxFileCount);
-// Parse the request
-List items = null;
-try {
-items = upload.parseRequest(multipartContext);
-} catch (final FileUploadException fue) {
-throw new IOException("Error parsing multipart request", fue);
-}
-this.parts = new ArrayList<>();
-for(final FileItem item : items) {
-this.parts.add(new PartImpl(item));
-}
-}
-
@Override
@SuppressWarnings({ "unchecked", "rawtypes" })
public Collection getParts() throws IOException, ServletException {
-return (Collection)checkMultipart();
-
+return null;
}
@Override
public Part getPart(String name) throws IOException, ServletException {
-Collection parts = this.checkMultipart();
-for(final Part p : parts) {
-if ( p.getName().equals(name) ) {
-return p;
-}
-}
Review Comment:
Similar to getParts this method should throw an exception
--
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: dev-unsubscr...@felix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
cziegeler commented on code in PR #367:
URL: https://github.com/apache/felix-dev/pull/367#discussion_r1912729481
##
http/base/src/main/java/org/apache/felix/http/base/internal/dispatch/ServletRequestWrapper.java:
##
@@ -411,143 +381,17 @@ public boolean isAsyncSupported()
return this.asyncSupported;
}
-private RequestContext getMultipartContext() {
-final RequestContext multipartContext;
-if (!POST_METHOD.equalsIgnoreCase(this.getMethod())) {
-multipartContext = null;
-} else {
-multipartContext = new RequestContext() {
-
-@Override
-public InputStream getInputStream() throws IOException {
-return ServletRequestWrapper.this.getInputStream();
-}
-
-@Override
-public String getContentType() {
-return ServletRequestWrapper.this.getContentType();
-}
-
-@Override
-public int getContentLength() {
-return ServletRequestWrapper.this.getContentLength();
-}
-
-@Override
-public String getCharacterEncoding() {
-return ServletRequestWrapper.this.getCharacterEncoding();
-}
-};
-}
-return multipartContext;
-}
-
-private Collection checkMultipart() throws IOException,
ServletException {
-if ( parts == null ) {
-final RequestContext multipartContext = getMultipartContext();
-if ( multipartContext != null &&
FileUploadBase.isMultipartContent(multipartContext) ) {
-if ( this.multipartConfig == null) {
-throw new IllegalStateException("Multipart not enabled for
servlet.");
-}
-
-if ( System.getSecurityManager() == null ) {
-handleMultipart(multipartContext);
-} else {
-final AccessControlContext ctx =
bundleForSecurityCheck.adapt(AccessControlContext.class);
-final IOException ioe = AccessController.doPrivileged(new
PrivilegedAction() {
-
-@Override
-public IOException run() {
-try {
-handleMultipart(multipartContext);
-} catch ( final IOException ioe) {
-return ioe;
-}
-return null;
-}
-}, ctx);
-if ( ioe != null ) {
-throw ioe;
-}
-}
-
-} else {
-throw new ServletException("Not a multipart request");
-}
-}
-return parts;
-}
-
-private void handleMultipart(final RequestContext multipartContext) throws
IOException {
-// Create a new file upload handler
-final FileUpload upload = new FileUpload();
-upload.setSizeMax(this.multipartConfig.multipartMaxRequestSize);
-upload.setFileSizeMax(this.multipartConfig.multipartMaxFileSize);
-upload.setFileItemFactory(new
DiskFileItemFactory(this.multipartConfig.multipartThreshold,
-new File(this.multipartConfig.multipartLocation)));
-upload.setFileCountMax(this.multipartConfig.multipartMaxFileCount);
-// Parse the request
-List items = null;
-try {
-items = upload.parseRequest(multipartContext);
-} catch (final FileUploadException fue) {
-throw new IOException("Error parsing multipart request", fue);
-}
-this.parts = new ArrayList<>();
-for(final FileItem item : items) {
-this.parts.add(new PartImpl(item));
-}
-}
-
@Override
@SuppressWarnings({ "unchecked", "rawtypes" })
public Collection getParts() throws IOException, ServletException {
-return (Collection)checkMultipart();
-
+return null;
Review Comment:
According to the servlet spec, this method either throws a ServletException
if the request is not a multipart request or always returns a collection. It
never returns null. Throwing an exception seems to be the right thing
--
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: dev-unsubscr...@felix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
paulrutter commented on PR #367: URL: https://github.com/apache/felix-dev/pull/367#issuecomment-2583412055 Thanks for clarifying @laeubi, that makes sense 👍 -- 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: dev-unsubscr...@felix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
laeubi commented on PR #367: URL: https://github.com/apache/felix-dev/pull/367#issuecomment-2583364930 > 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. -- 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: dev-unsubscr...@felix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
paulrutter commented on PR #367: URL: https://github.com/apache/felix-dev/pull/367#issuecomment-2583345146 I see, but wouldn't it make sense to mark the dependencies as optional as well, in https://github.com/apache/felix-dev/blob/f572bd8131df53e1fd39e9655afe6c7758541cc1/http/jetty/pom.xml#L504? -- 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: dev-unsubscr...@felix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
stbischof commented on PR #367: URL: https://github.com/apache/felix-dev/pull/367#issuecomment-2583338357 yes it is marked as optional. so 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 -- 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: dev-unsubscr...@felix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [http] extract Multipart code into new ServletRequestMultipartWrapper [felix-dev]
paulrutter commented on PR #367: URL: https://github.com/apache/felix-dev/pull/367#issuecomment-2583304211 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? -- 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: dev-unsubscr...@felix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
