FELIX-6829 Remove http.jetty dependency on commons-fileupload

[enapps-enorman]

Jetty does not need the Apache commons-fileupload library. Modern versions of Jetty support the Java Servlet Specification (3.0 and newer), which includes built-in, native APIs to parse multipart/form-data requests.

Refactoring to use the standard servlet apis:

1. remove the commons-fileupload dependency
2. Change ServletRequestMultipartWrapper to delegate the multipart request handling to the jetty servlet container
3. Other refactoring to replace the org.apache.commons.fileupload references with the servlet Part equivalent

[enapps-enorman]

I believe the now unused ServletHandler#getMultipartSecurityContext and related code could also be removed since the only usage was in the now removed multipart handling code.

But I thought I would get some feedback on the general approach before doing that extra cleanup.

UPDATE: cleaned up the the dead code at 3627401

[paulrutter]

@enapps-enorman I like the approach, it's just another dependency we can do without nowadays. I'm fine with removing and delegation to Jetty instead.

[cziegeler]

I suggest to run the OSGi TCK for the http service, http whiteboard and servlet whiteboard on the proposed change.

[enapps-enorman]

I suggest to run the OSGi TCK

For clarification, is that referring to the test cases published at https://github.com/osgi/osgi or something private I don't have access to? If it is the former then I tried running it against org.apache.felix.http.jetty-5.2.3-SNAPSHOT and I didn't get any test failures.

[cziegeler]

Yes, it is that public git repo, you need to run the test cases from the R8 cmpn release:

• https://github.com/osgi/osgi/tree/r8-cmpn-final/org.osgi.test.cases.http
• https://github.com/osgi/osgi/tree/r8-cmpn-final/org.osgi.test.cases.http.whiteboard
• https://github.com/osgi/osgi/tree/r8-cmpn-final/org.osgi.test.cases.http.whiteboard.secure

and R8.1 cmpn release:

• https://github.com/osgi/osgi/tree/8.1.0.cmpn/org.osgi.test.cases.servlet
• https://github.com/osgi/osgi/tree/8.1.0.cmpn/org.osgi.test.cases.servlet.secure

[enapps-enorman]

Yes, it is that public git repo, you need to run the test cases from the R8 cmpn release:

@cziegeler Ok. Unfortunately running the test cases off those branches fails for all felix.http.jetty versions above 5.x with what looks like some resolution problems with missing slf4j packages? I'm not familiar with this codebase at all, so if you have some hints on what and where is the correct places to make adjustments to make the felix.http.jetty 5.x versions function I would appreciate it. For example, felix.http.jetty 4.2.32 is the last version that seems to work, but 5.0.0 does not.

[cziegeler]

Its too long ago to remember the details, but I think it required to add some additional bundles in some of these library files. If the main branch works for you, you could also just try to move the test sub projects from those branches into the main branch and execute it there. I think thats what I did last time...
We also have https://felix.apache.org/documentation/development/using-the-osgi-compliance-tests.html - but thats completely outdated and doesnt help at all.

[enapps-enorman]

Its too long ago to remember the details

@cziegeler Ok. I got some hints from copilot and I think I figured out what to change to get the tests to run. Below is a summary of the results. NOTE: There are some tests failing but from what I can tell it is the same set of tests that also fail with http.jetty:5.2.2. I can try to take a closer look at the failures when I get some free time. But maybe those should be tracked separately from this one?

With these changes: https://github.com/osgi/osgi/compare/r8-cmpn-final...enapps-enorman:osgi:r8-cmpn-final-jetty5.2.3?expand=1

r8-cmpn-final/org.osgi.test.cases.http

	Test run finished after 504 ms
	[         5 containers found      ]
	[         0 containers skipped    ]
	[         5 containers started    ]
	[         0 containers aborted    ]
	[         5 containers successful ]
	[         0 containers failed     ]
	[        29 tests found           ]
	[         0 tests skipped         ]
	[        29 tests started         ]
	[         0 tests aborted         ]
	[        29 tests successful      ]
	[         0 tests failed          ]


r8-cmpn-final/org.osgi.test.cases.http.whiteboard

	Test run finished after 6583 ms
	[        12 containers found      ]
	[         0 containers skipped    ]
	[        12 containers started    ]
	[         0 containers aborted    ]
	[        12 containers successful ]
	[         0 containers failed     ]
	[       159 tests found           ]
	[         0 tests skipped         ]
	[       159 tests started         ]
	[         0 tests aborted         ]
	[       149 tests successful      ]
	[        10 tests failed          ]


r8-cmpn-final/org.osgi.test.cases.http.whiteboard.secure

	Test run finished after 2291 ms
	[         4 containers found      ]
	[         0 containers skipped    ]
	[         4 containers started    ]
	[         0 containers aborted    ]
	[         4 containers successful ]
	[         0 containers failed     ]
	[         6 tests found           ]
	[         0 tests skipped         ]
	[         6 tests started         ]
	[         0 tests aborted         ]
	[         5 tests successful      ]
	[         1 tests failed          ]

With these changes: https://github.com/osgi/osgi/compare/8.1.0.cmpn...enapps-enorman:osgi:8.1.0.cmpn-jetty5.2.3?expand=1

8.1.0.cmpn/org.osgi.test.cases.servlet

	Test run finished after 6620 ms
	[        12 containers found      ]
	[         0 containers skipped    ]
	[        12 containers started    ]
	[         0 containers aborted    ]
	[        12 containers successful ]
	[         0 containers failed     ]
	[       147 tests found           ]
	[         0 tests skipped         ]
	[       147 tests started         ]
	[         0 tests aborted         ]
	[       146 tests successful      ]
	[         1 tests failed          ]



8.1.0.cmpn/org.osgi.test.cases.servlet.secure

	Test run finished after 4325 ms
	[         4 containers found      ]
	[         0 containers skipped    ]
	[         4 containers started    ]
	[         0 containers aborted    ]
	[         4 containers successful ]
	[         0 containers failed     ]
	[         6 tests found           ]
	[         0 tests skipped         ]
	[         6 tests started         ]
	[         0 tests aborted         ]
	[         6 tests successful      ]
	[         0 tests failed          ]

[cziegeler]

@enapps-enorman Thanks! If the same set of tests failed already before this change, then we should track those separately. But we need to make sure to pass all of them again, initially we did.
Its good that we can get rid of commons-fileupload, I tried a similar approach years ago with jetty 9 and couldnt succeed.