[SP-5013] Backport of PPP-4281 - Use of Vulnerable Component: postgre…

…sql-42.1.1.jar (CVE-2018-10936) (8.2 Suite)
@pentaho/tatooine @pentaho-lmartins
Depends on https://github.com/pentaho/maven-parent-poms/pull/126

Build Failed
:x: Something went wrong while validating this pull request.
Build Commands
mvn -B -f 'pom.xml' -pl 'engine' -P '!assemblies' -Dsurefire.runOrder=alphabetical -Daudit -amd clean install

Stdout log
(last 100 lines)
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/pax/url/pax-url-aether/2.3.0/pax-url-aether-2.3.0.pom (12 kB at 325 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.spring/3.0.3/org.apache.karaf.deployer.spring-3.0.3.pom
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.spring/3.0.3/org.apache.karaf.deployer.spring-3.0.3.pom (4.3 kB at 215 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/deployer/3.0.3/deployer-3.0.3.pom
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/deployer/3.0.3/deployer-3.0.3.pom (1.7 kB at 53 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.blueprint/3.0.3/org.apache.karaf.deployer.blueprint-3.0.3.pom
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.blueprint/3.0.3/org.apache.karaf.deployer.blueprint-3.0.3.pom (4.1 kB at 59 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.features/3.0.3/org.apache.karaf.deployer.features-3.0.3.pom
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.features/3.0.3/org.apache.karaf.deployer.features-3.0.3.pom (4.3 kB at 143 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.kar/3.0.3/org.apache.karaf.deployer.kar-3.0.3.pom
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.kar/3.0.3/org.apache.karaf.deployer.kar-3.0.3.pom (4.0 kB at 330 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/kar/org.apache.karaf.kar.core/3.0.3/org.apache.karaf.kar.core-3.0.3.pom
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/kar/org.apache.karaf.kar.core/3.0.3/org.apache.karaf.kar.core-3.0.3.pom (4.2 kB at 222 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/kar/kar/3.0.3/kar-3.0.3.pom
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/kar/kar/3.0.3/kar-3.0.3.pom (1.6 kB at 76 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/shell/org.apache.karaf.shell.console/3.0.3/org.apache.karaf.shell.console-3.0.3.pom
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/shell/org.apache.karaf.shell.console/3.0.3/org.apache.karaf.shell.console-3.0.3.pom (8.8 kB at 283 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/shell/shell/3.0.3/shell-3.0.3.pom
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/shell/shell/3.0.3/shell-3.0.3.pom (1.7 kB at 46 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/jaas/org.apache.karaf.jaas.modules/3.0.3/org.apache.karaf.jaas.modules-3.0.3.pom
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/jaas/org.apache.karaf.jaas.modules/3.0.3/org.apache.karaf.jaas.modules-3.0.3.pom (5.8 kB at 240 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/sshd/sshd-core/0.12.0/sshd-core-0.12.0.pom
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/sshd/sshd-core/0.12.0/sshd-core-0.12.0.pom (8.5 kB at 117 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/sshd/sshd/0.12.0/sshd-0.12.0.pom
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/sshd/sshd/0.12.0/sshd-0.12.0.pom (12 kB at 216 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/mina/mina-core/2.0.7/mina-core-2.0.7.pom
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/mina/mina-core/2.0.7/mina-core-2.0.7.pom (1.8 kB at 47 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/mina/mina-parent/2.0.7/mina-parent-2.0.7.pom
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/mina/mina-parent/2.0.7/mina-parent-2.0.7.pom (30 kB at 666 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/felix/org.apache.felix.fileinstall/3.4.2/org.apache.felix.fileinstall-3.4.2.jar
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/features/org.apache.karaf.features.core/3.0.3/org.apache.karaf.features.core-3.0.3.jar
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/tooling/karaf-maven-plugin/3.0.3/karaf-maven-plugin-3.0.3.jar
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/pax/url/pax-url-wrap/2.3.0/pax-url-wrap-2.3.0-uber.jar
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/base/ops4j-base-net/1.4.0/ops4j-base-net-1.4.0.jar
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/features/org.apache.karaf.features.core/3.0.3/org.apache.karaf.features.core-3.0.3.jar (147 kB at 3.3 MB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/base/ops4j-base-monitors/1.4.0/ops4j-base-monitors-1.4.0.jar
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/felix/org.apache.felix.fileinstall/3.4.2/org.apache.felix.fileinstall-3.4.2.jar (104 kB at 1.2 MB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/pax/swissbox/pax-swissbox-bnd/1.8.0/pax-swissbox-bnd-1.8.0.jar
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/base/ops4j-base-net/1.4.0/ops4j-base-net-1.4.0.jar (12 kB at 145 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/pax/url/pax-url-commons/2.3.0/pax-url-commons-2.3.0.jar
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/tooling/karaf-maven-plugin/3.0.3/karaf-maven-plugin-3.0.3.jar (153 kB at 1.3 MB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/pax/swissbox/pax-swissbox-property/1.8.0/pax-swissbox-property-1.8.0.jar
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/pax/url/pax-url-commons/2.3.0/pax-url-commons-2.3.0.jar (9.2 kB at 70 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/pax/url/pax-url-aether/2.3.0/pax-url-aether-2.3.0.jar
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/base/ops4j-base-monitors/1.4.0/ops4j-base-monitors-1.4.0.jar (8.0 kB at 58 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.spring/3.0.3/org.apache.karaf.deployer.spring-3.0.3.jar
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.spring/3.0.3/org.apache.karaf.deployer.spring-3.0.3.jar (31 kB at 167 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.blueprint/3.0.3/org.apache.karaf.deployer.blueprint-3.0.3.jar
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/pax/url/pax-url-wrap/2.3.0/pax-url-wrap-2.3.0-uber.jar (1.2 MB at 6.0 MB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.features/3.0.3/org.apache.karaf.deployer.features-3.0.3.jar
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/pax/swissbox/pax-swissbox-bnd/1.8.0/pax-swissbox-bnd-1.8.0.jar (9.8 kB at 53 kB/s)
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/pax/swissbox/pax-swissbox-property/1.8.0/pax-swissbox-property-1.8.0.jar (4.0 kB at 21 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/kar/org.apache.karaf.kar.core/3.0.3/org.apache.karaf.kar.core-3.0.3.jar
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.kar/3.0.3/org.apache.karaf.deployer.kar-3.0.3.jar
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/kar/org.apache.karaf.kar.core/3.0.3/org.apache.karaf.kar.core-3.0.3.jar (44 kB at 212 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/shell/org.apache.karaf.shell.console/3.0.3/org.apache.karaf.shell.console-3.0.3.jar
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.features/3.0.3/org.apache.karaf.deployer.features-3.0.3.jar (31 kB at 148 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/jaas/org.apache.karaf.jaas.modules/3.0.3/org.apache.karaf.jaas.modules-3.0.3.jar
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.blueprint/3.0.3/org.apache.karaf.deployer.blueprint-3.0.3.jar (31 kB at 146 kB/s)
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/deployer/org.apache.karaf.deployer.kar/3.0.3/org.apache.karaf.deployer.kar-3.0.3.jar (13 kB at 60 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/mina/mina-core/2.0.7/mina-core-2.0.7.jar
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/sshd/sshd-core/0.12.0/sshd-core-0.12.0.jar
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/shell/org.apache.karaf.shell.console/3.0.3/org.apache.karaf.shell.console-3.0.3.jar (287 kB at 1.1 MB/s)
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/mina/mina-core/2.0.7/mina-core-2.0.7.jar (645 kB at 2.3 MB/s)
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/sshd/sshd-core/0.12.0/sshd-core-0.12.0.jar (622 kB at 2.2 MB/s)
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/apache/karaf/jaas/org.apache.karaf.jaas.modules/3.0.3/org.apache.karaf.jaas.modules-3.0.3.jar (1.2 MB at 4.2 MB/s)
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/ops4j/pax/url/pax-url-aether/2.3.0/pax-url-aether-2.3.0.jar (2.9 MB at 9.7 MB/s)
[ERROR] [ERROR] Some problems were encountered while processing the POMs:
[ERROR] 'dependencies.dependency.version' for org.postgresql:postgresql:jar is missing. @ line 487, column 17
 @
[ERROR] The build could not read 1 project -> [Help 1]
[ERROR]
[ERROR]   The project pentaho-kettle:kettle-engine:<IP_ADDRESS>-SNAPSHOT (/home/buildguy/project/engine/pom.xml) has 1 error
[ERROR]     'dependencies.dependency.version' for org.postgresql:postgresql:jar is missing. @ line 487, column 17
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/ProjectBuildingException
[INFO] Scanning for projects...
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/pentaho/pentaho-ce-jar-parent-pom/<IP_ADDRESS>-SNAPSHOT/maven-metadata.xml
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/pentaho/pentaho-ce-jar-parent-pom/<IP_ADDRESS>-SNAPSHOT/maven-metadata.xml (839 B at 4.1 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/pentaho/pentaho-ce-parent-pom/<IP_ADDRESS>-SNAPSHOT/maven-metadata.xml
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/pentaho/pentaho-ce-parent-pom/<IP_ADDRESS>-SNAPSHOT/maven-metadata.xml (835 B at 31 kB/s)
[INFO] Downloading from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/pentaho/pentaho-parent-pom/<IP_ADDRESS>-SNAPSHOT/maven-metadata.xml
[INFO] Downloaded from pentaho-public: http://nexus.pentaho.org/content/groups/omni/org/pentaho/pentaho-parent-pom/<IP_ADDRESS>-SNAPSHOT/maven-metadata.xml (614 B at 38 kB/s)
[ERROR] [ERROR] Some problems were encountered while processing the POMs:
[ERROR] 'dependencies.dependency.version' for org.postgresql:postgresql:jar is missing. @ line 487, column 17
 @
[ERROR] The build could not read 1 project -> [Help 1]
[ERROR]
[ERROR]   The project pentaho-kettle:kettle-engine:<IP_ADDRESS>-SNAPSHOT (/home/buildguy/project/engine/pom.xml) has 1 error
[ERROR]     'dependencies.dependency.version' for org.postgresql:postgresql:jar is missing. @ line 487, column 17
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/ProjectBuildingException

Stderr log
(last 100 lines)
java.lang.Exception: org.pentaho.build.buddy.bundles.orchestrator.OrchestratorImpl$BuildException: java.io.IOException: Head build returned with code 1
	at org.pentaho.build.buddy.bundles.orchestrator.OrchestratorImpl.orchestrate(OrchestratorImpl.java:381)
	at Proxye162dfd3_b29b_40a2_b7b0_a76f47dc915d.orchestrate(Unknown Source)
	at org.pentaho.build.buddy.bundles.rest.OrchestratorRestService$1.write(OrchestratorRestService.java:62)
	at org.apache.cxf.jaxrs.provider.BinaryDataProvider.writeTo(BinaryDataProvider.java:172)
	at org.apache.cxf.jaxrs.utils.JAXRSUtils.writeMessageBody(JAXRSUtils.java:1381)
	at org.apache.cxf.jaxrs.interceptor.JAXRSOutInterceptor.serializeMessage(JAXRSOutInterceptor.java:244)
	at org.apache.cxf.jaxrs.interceptor.JAXRSOutInterceptor.processResponse(JAXRSOutInterceptor.java:120)
	at org.apache.cxf.jaxrs.interceptor.JAXRSOutInterceptor.handleMessage(JAXRSOutInterceptor.java:83)
	at org.apache.cxf.phase.PhaseInterceptorChain.doIntercept(PhaseInterceptorChain.java:308)
	at org.apache.cxf.interceptor.OutgoingChainInterceptor.handleMessage(OutgoingChainInterceptor.java:83)
	at org.apache.cxf.phase.PhaseInterceptorChain.doIntercept(PhaseInterceptorChain.java:308)
	at org.apache.cxf.transport.ChainInitiationObserver.onMessage(ChainInitiationObserver.java:121)
	at org.apache.cxf.transport.http.AbstractHTTPDestination.invoke(AbstractHTTPDestination.java:253)
	at org.apache.cxf.transport.servlet.ServletController.invokeDestination(ServletController.java:234)
	at org.apache.cxf.transport.servlet.ServletController.invoke(ServletController.java:208)
	at org.apache.cxf.transport.servlet.ServletController.invoke(ServletController.java:160)
	at org.apache.cxf.transport.servlet.CXFNonSpringServlet.invoke(CXFNonSpringServlet.java:180)
	at org.apache.cxf.transport.servlet.AbstractHTTPServlet.handleRequest(AbstractHTTPServlet.java:298)
	at org.apache.cxf.transport.servlet.AbstractHTTPServlet.doPost(AbstractHTTPServlet.java:217)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:707)
	at org.apache.cxf.transport.servlet.AbstractHTTPServlet.service(AbstractHTTPServlet.java:273)
	at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:812)
	at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:587)
	at org.ops4j.pax.web.service.jetty.internal.HttpServiceServletHandler.doHandle(HttpServiceServletHandler.java:71)
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:143)
	at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:577)
	at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:223)
	at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1127)
	at org.ops4j.pax.web.service.jetty.internal.HttpServiceContext.doHandle(HttpServiceContext.java:287)
	at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:515)
	at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:185)
	at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1061)
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141)
	at org.ops4j.pax.web.service.jetty.internal.JettyServerHandlerCollection.handle(JettyServerHandlerCollection.java:80)
	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:97)
	at org.eclipse.jetty.server.Server.handle(Server.java:499)
	at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:311)
	at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:257)
	at org.eclipse.jetty.io.AbstractConnection$2.run(AbstractConnection.java:544)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:635)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$3.run(QueuedThreadPool.java:555)
	at java.lang.Thread.run(Thread.java:748)
Caused by: org.pentaho.build.buddy.bundles.orchestrator.OrchestratorImpl$BuildException: java.io.IOException: Head build returned with code 1
	at org.pentaho.build.buddy.bundles.orchestrator.OrchestratorImpl.doOrchestrate(OrchestratorImpl.java:609)
	at org.pentaho.build.buddy.bundles.orchestrator.OrchestratorImpl.orchestrate(OrchestratorImpl.java:364)
	... 41 more
Caused by: java.io.IOException: Head build returned with code 1
	... 43 more

Build Completed
:x: This pull request has errors. They will need to be addressed before it can be accepted. See below for more details. Some links are also available below for further assistance in addressing those issues.
Build Commands
mvn -B -f 'pom.xml' -pl 'engine' -P '!assemblies' -Dsurefire.runOrder=alphabetical -Daudit -amd clean install

Cleanup Commands
mvn -B -f 'pom.xml' -pl 'engine' -P '!assemblies' -amd build-helper:remove-project-artifact

Changed files
  engine/pom.xml

Newly Broken Tests:
org.pentaho.di.trans.step.jms.JmsStreamSourceTest.testReceiveMessage:
:red_circle: java.lang.Exception
java.lang.Exception: test timed out after 5000 milliseconds
	at org.pentaho.di.trans.step.jms.JmsStreamSourceTest.testReceiveMessage(JmsStreamSourceTest.java:90)

Unit Test Coverage
These statistics help you identify how your changes have affected the coverage of the following files. If a file is not in this list, then its coverage was not affected by your changes. To get some help interpreting these metrics, please refer to Jacoco's documentation.

:warning: Coverage Changes (click to expand)
org.pentaho.di.job.entries.evalfilesmetrics.JobEntryEvalFilesMetrics

Branch Change: -0.4% :small_red_triangle_down:
Complexity Change: -0.5% :small_red_triangle_down:
Instruction Change: -0.1% :small_red_triangle_down:

org.pentaho.di.trans.steps.dbproc.DBProcMeta

Instruction Change: -0.1% :small_red_triangle_down:

org.pentaho.di.trans.steps.dimensionlookup.DimensionLookupMeta

Branch Change: -1.8% :small_red_triangle_down:
Complexity Change: -1.1% :small_red_triangle_down:
Instruction Change: -0.4% :small_red_triangle_down:
Line Change: -0.3% :small_red_triangle_down:

org.pentaho.di.trans.steps.ldapoutput.LDAPOutputMeta

Instruction Change: -0.1% :small_red_triangle_down:

org.pentaho.di.trans.steps.sort.SortRowsMeta

Instruction Change: -0.1% :small_red_triangle_down:

org.pentaho.di.trans.steps.sortedmerge.SortedMergeMeta

Instruction Change: -0.2% :small_red_triangle_down:

org.pentaho.di.trans.steps.gpload.GPLoadMeta

Branch Change: -0.9% :small_red_triangle_down:
Complexity Change: -0.8% :small_red_triangle_down:
Instruction Change: -0.2% :small_red_triangle_down:

Build Completed
:fire: This pull request has some issues. It would be preferable to fix them in order for it to be just perfect. See below for more details. Some links are also available below for further assistance in addressing those issues.
Build Commands
mvn -B -f 'pom.xml' -pl 'engine' -P '!assemblies' -Dsurefire.runOrder=alphabetical -Daudit -amd clean install

Cleanup Commands
mvn -B -f 'pom.xml' -pl 'engine' -P '!assemblies' -amd build-helper:remove-project-artifact

Changed files
  engine/pom.xml

Unit Test Coverage
These statistics help you identify how your changes have affected the coverage of the following files. If a file is not in this list, then its coverage was not affected by your changes. To get some help interpreting these metrics, please refer to Jacoco's documentation.

:warning: Coverage Changes (click to expand)
org.pentaho.di.job.entries.evalfilesmetrics.JobEntryEvalFilesMetrics

Branch Change: -0.4% :small_red_triangle_down:
Complexity Change: -0.5% :small_red_triangle_down:
Instruction Change: -0.1% :small_red_triangle_down:

org.pentaho.di.job.entries.evaluatetablecontent.JobEntryEvalTableContent

Branch Change: -1.1% :small_red_triangle_down:
Complexity Change: -1.1% :small_red_triangle_down:
Instruction Change: -0.1% :small_red_triangle_down:

org.pentaho.di.job.entries.sql.JobEntrySQL

Instruction Change: -3.4% :small_red_triangle_down:
Line Change: -3.8% :small_red_triangle_down:

org.pentaho.di.trans.steps.datagrid.DataGridMeta

Branch Change: -2% :small_red_triangle_down:
Complexity Change: -1.7% :small_red_triangle_down:
Instruction Change: -0.2% :small_red_triangle_down:

org.pentaho.di.trans.steps.dbproc.DBProcMeta

Instruction Change: -0.1% :small_red_triangle_down:

org.pentaho.di.trans.steps.dimensionlookup.DimensionLookupMeta

Branch Change: -1.8% :small_red_triangle_down:
Complexity Change: -1.1% :small_red_triangle_down:
Instruction Change: -0.4% :small_red_triangle_down:
Line Change: -0.3% :small_red_triangle_down:

org.pentaho.di.trans.steps.memgroupby.MemoryGroupByMeta

Branch Change: -4.4% :small_red_triangle_down:
Complexity Change: -5.2% :small_red_triangle_down:
Instruction Change: -0.1% :small_red_triangle_down:
Line Change: -0.4% :small_red_triangle_down:

org.pentaho.di.trans.steps.replacestring.ReplaceStringMeta

Instruction Change: -0.1% :small_red_triangle_down:

org.pentaho.di.trans.steps.sort.SortRowsMeta

Instruction Change: -0.1% :small_red_triangle_down:

org.pentaho.di.trans.steps.uniquerows.UniqueRowsMeta

Instruction Change: -0.1% :small_red_triangle_down:


bugfix(TDI-38561) : manage wrong endpoint in REST connection

What is the current behavior? (You can also link to an open issue here)
https://jira.talendforge.org/browse/TDI-38561
What is the new behavior?
Fixed.
Please check if the PR fulfills these requirements

[X] The commit message follows Talend standard
[X] Tests for the changes have been added (for bug fixes / features)
[ ] Docs have been added / updated (for bug fixes / features) ?
[ ] The code coverage on new code >75%
[X] The new code does not introduce new technical issues (sonar / eslint)

What kind of change does this PR introduce?

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build / CI related changes
[ ] Other... Please describe:

Does this PR introduce a breaking change?

[ ] Yes
[X] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information:

Pull request test result: success :star:


Version selection loading the new content

I have a question about the code. I am trying to understand when we click on a version how does it load the generated website for that specific version? I looked at the menu package but it looks like it just updates the menu but I don't know how it loads the generated website (How does it update the navigation links) for that specific version . Would you please give me some pointers?

Structor generates the following files structure:
. (latest, branch v1.2)
├── ...
├── index.html
├── v1.0 (branch v1.0)
│   ├── ...
│   └── index.html
├── v1.1 (branch v1.1)
│   ├── ...
│   └── index.html
└── v1.2 (branch v1.2)
    ├── ...
    └── index.html

So if you serve the content from . to mydoc.com

mydoc.com (latest, branch v1.2)
mydoc.com/v1.0 (branch v1.0)
mydoc.com/v1.1 (branch v1.1)
mydoc.com/v1.2 (branch v1.2)

The "loading" is just a click on a simple HTML anchor.
https://github.com/containous/structor/blob/7352c07ef4e6581e078daa2259fceeda2123bc0e/traefik-menu.js.gotmpl#L64-L76

Thanks for your response and the pointer.

@ldez
Sorry for my lots of questions. I am trying to reproduce almost the same thing locally except I use tags instead of branches. A couple of things are not clear for me yet:
1- Do we need to build each version using the modified version of mkdocs.yml file (i.e. the version which contains path to the menu js file (extra_javascript))? If that is the case how the navigation will get updated?
2- I can build the website for each version without any problems but when I select one of them, it doesn't reload it because it always use the mkdocs.yml file in the root which points to the latest version.I tried to build your site locally to see how it works but it looks like it doesn't work for local testing (i.e. it doesn't show any version selector).

The branches are a requirement, Structor doesn't follow tags.
Structor modifies the mkdocs.yml file during the build, so you don't need to do that manually.
To use structor you don't need to adapt your mkdocs.yml, but you need to create a traefik-menu.js.gotmpl to generate the menu related to your theme.


Tests around server-utils

Having tests around the server-utils directory would be good starting place to add tests as it would help avoid regressions when refactoring and adding new features and provide a framework for testing new features.
The server-utils directory is a good place to start because it contains logic in locations like:

transformation of the markdown to html (in convertToQuestion) https://github.com/StampyAI/stampy-ui/blob/d6c6a7b564fd4e81f38234ff0d2322dda762f4e3/app/server-utils/stampy.ts#L215
kv-cache

@Aprillion @mruwnik , my current vision for this is to cover the "load" in stampy.ts functions like stampy::loadQuestionDetail and stampy::loadInitialQuestions. Does that make sense in your view?
To do this, I think separating the functional code from the requests to coda would be helpful. This would allow the requests to coda to be mocked or injected so that there wouldn't be a need to maintain a coda instance in order to test the functional code.
Mocking could be done with the help of library like Jest Mocks: https://jestjs.io/docs/mock-functions
Any thoughts on the above?

if you want to start testing functions that talk to the API, I would recommend using https://mswjs.io/ to mock the server responses from Coda, not just mock functions..

Yup, the fewer and futher back the mocks, the better. Maybe start with setting up the structure for unit tests (plus one or two to make sure it works) and then proceed to testing the coda stuff?

Thanks for the direction @Aprillion and @mruwnik 👍 . Draft PR here: https://github.com/StampyAI/stampy-ui/pull/193
I decided to focus on unit tests for urlToIframe as not Coda mocking is required.

With #193 merged, I'll take a look at https://mswjs.io/ for mocking the server responses.

Hi @mruwnik and @Aprillion . I've realized that to test the code in server-utils, it would be helpful to mock both Cloudflare KV and Coda responses. My thinking at the moment is to use:

msw for mocking Coda
miniflare for mocking Cloudflare KV

msw was discussed previously but what is your opinion on miniflare?
Though it means an additional dependency (in addition to msw), it seems to me like it could be a good fit to more easily mock the KV calls (e.g. STAMPY_KV.get(key)). Otherwise, I think one might need to dig into the internals of the KV client to know which HTTP requests to mock.

miniflare or something is already used by remix to run localhost, so however that works it can be reused for tests IMHO

Oh yea, you're right, miniflare is already a dev dependancy. I hadn't noticed that. I guess it doesn't hurt to give it a go to reuse it for tests then 👍

I found some discussion about testing remix which I'm going through to see if there's anything relevant to stampy-ui's case:

https://github.com/remix-run/remix/discussions/2481
https://github.com/remix-run/remix/issues/372

There is a @remix-run/testing package that might be useful here: https://www.npmjs.com/package/@remix-run/testing?activeTab=readme

I think the above discussions on remix repo and @remix-run/testing are more for testing react components that have dependency on remix, without needing to run remix. As such, I think they are not a relevant to the goal of "tests around server-utils".
I will proceed with the MSW + miniflare approach for now (see draft PR above for the very start of this).

@Aprillion I'm not too far with the implementation (and I'm travelling until the end of May so probably won't make much progress till after then 😅). But I will definitely check out the new MSW release, thanks. I think that direct support for Fetch API could be useful 👍

@Aprillion it's JohnH#0114, I added him to team

I've made a bit of progress on this task.
I discovered the Jest environment provided by Miniflare: https://miniflare.dev/testing/jest
I think it could be a promising approach. I have a WIP draft PR here: https://github.com/jrhender/stampy-ui/pull/2
Thus far, the PR is using undici, rather than msw, to mock the requests to coda. The msw mock wasn't working in during my initial attempt to run a test wile undici integration is provided out-of-the-box from the miniflare jest env (https://miniflare.dev/testing/jest#mocking-outbound-fetch-requests) and seems to work well. I'm still open to msw, but I figure might as well use undici as that is presumably tested by the miniflare team for use with their jest env.

Initial tests now merged, let's have a separate issue for future improvements when needed.


Include arm32 in artifacts

Hi,
currently there is no artifact for arm32 devices even though the build pipelines are basically ready to go. Are you planning on integrating this in the near future or is there more work that needs to be done with that?

@thegreatdane6 : We will be adding support for ARM32 with our next release

We have added support for Debian 10 ARM 32 with our latest release.
If you need support for other OS flavors, please add a new suggestion here: Device Update Ideas for consideration


Suggestion: check setttings-file for "read-only"

The software should report to the user that "xmlnotepad.settings" is set to "read-only" if it is so. Now the software does not report, does not save - this could cause confusion....

I pushed a fix for this, it will show up in the next version (<IP_ADDRESS>).  When it tries to save settings, if it fails you will be notified like this, it will be up to you to figure out what you want to do to fix it, but at least you will know the save did not work as expected.


chore(release): 26.1.2

Release v26.1.2

Pull Request Test Coverage Report for Build 5344

0 of 0   changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 92.721%

Totals

Change from base Build 5343:
0.0%

Covered Lines:
1056

Relevant Lines:
1091

💛  - Coveralls


Add support for advanced usage using the controller pattern

Split from #116
POC branch: https://github.com/tusdotnet/tusdotnet/tree/POC/controller-pattern
This issue needs refinement. See earlier discussions in #116 . When implementing this issue we also need to add support for dependency injection, i.e. services.AddTus().

Ive worked on the POC branch and changed a few things:

Removed ITusConfigurator
Simplified Configuration
Add Termination, CreationWithUpload, CreationDeferLength support
Changed a few things about TusController

The minimal configuration to register a Tus Controller is now:
services.AddTus();
---
endpoints.MapController<MyTusController>("/files");

You can view it here:
POC/controller-pattern-better-configuration
It currently doent validate requests but that can be added in the future.
Btw the "EndpointRouting" folder is getting kinda bloated but i dont know where else to put the files :|
Let me know what you think of this :)

That's a massive amount of work you put in! :) I'll have a look during the week and will get back to you.

Overall this looks very promising but I think it still needs some details ironed out. You have done a tremendous amount of work! As I wrote in the other issue I think this type of configuration is somewhat of a niche way of doing things. Most people will just want to know when the file is completed so they can continue with their business logic. With that being said, I think there is still a place for more complex scenarios where one really needs fine grained control of the protocol and the code.
There are many things I like with your implementation, specifically the feel of a regular ASP.NET controller, how you split the DefaultTusConfiguration into several classes and that you changed the DetermineIntent method to return a IntentType instead of the handler.
There are some things that I don't like that I will discuss below. I know that this might seem a bit negative which is not my intent. I have been working on this project since 2016 and know most ins and outs of the code and protocol and I want to have the best possible outcome of this change. If you thing I have misunderstood something please feel free to correct me. Also the comments below are not sorted or prioritized :)
From a setup perspective I think we could go a bit more "complex" while still having support for both this issue and #147. I am imagining that the setup would look something like this:

services.AddTus() // Adds generic things that we need, HttpContextAccessor, StorageService etc.
        .WithStorage(httpContext => new TusDiskStore(...)) // Configure the storage to use. HttpContext allows us to use different stores for different endpoints, users, whatever, or just use a single store. We might want to add an overload that just takes a ITusStore?
        .WithDefaultOptions(new TusOptions { // Configure default options to use if nothing else is specified on the endpoint. This object contains all options not specifically related to how to store files but rather to what type of files we want to store. This class should follow the options pattern (https://docs.microsoft.com/en-us/aspnet/core/fundamentals/configuration/options?view=aspnetcore-5.0)
            Expiration = DateTime.FromMinutes(100),
            ExpirationType = ExpirationType.Sliding,
            MaxAllowedUploadSizeInBytesLong = ...,
            MetadataParsingStrategy ...
        })
        .AddController<MyTusController>() // Adds the specific controller to avoid assembly scanning. We might want to add something like AddControllersFromAssemblyOf<MyTusController>() which does the same thing your code does.

...

app.UseRouting();
app.UseEndpoints(endpoints => {

    // Uses MyTusController with the default settings and the storage configured
    endpoints.MapTus<MyTusController>("/videos").RequireAuthorization();

    // Uses a built in "EventsBasedController" that runs the correct event at the correct time.
    // In early version of #147 this could just route everything through the intentbased handler.
    endpoints.MapTus("/images", new Events { OnFileCompleteAsync = ... })
             .WithOptions(new TusOptions{...}) // Overrides options provided for this specific endpoint. Maybe even allow to run WithStorage(httpContext => ...) to override the default one.
            .RequireAuthorization();
});

In the above setup the TusStorageService always contains the store (storage proxy probably) to use so there is no need for the endpoint to care. Compare this to how you would setup a EF context or a Redis connection. This would also solve the TODO in public virtual Task<ControllerCapabilities> GetCapabilities().
Since the TusStorageService is "complete" there is no need for the [TusInheritCapabilities(typeof(TusDiskStore))] attribute. I think that the TusEnableExtensionAttribute and TusDisableExtensionAttribute are a good idea but not implemented correctly. These will only not announce the functionality but the functionality will still be there. I think we can postpone this feature as there haven't been any real traction for the idea. If we need it further down it should be implemented in the storage service.
I'm not really sure what the usage is for the [TusController] attribute? It seems redundant as the controller must inherit from TusControllerBase anyway. We could probably rename TusControllerBase to just TusController.
Speaking of TusController. I'm not a fan of the abstract methods forcing users to implement each and every method to use this pattern. I can imagine that some users might want to just override a specific method, e.g. they want to add the file id and the current user id to a DB table and thus just wants to override the Create method. I like that you added FileCompleted to the tus base controller so that users can override only this method.
Given the above, we could probably change how we inject the StorageService to be the same as e.g. HttpContext (property injection). This change, together with changing methods from abstract to virtual and the storage service containing the actual store to use, would allow us to write this code:
 [Authorize(Policy = "create-file-policy")]
public override async Task<ITusCreateActionResult> Create(CreateContext context, CancellationToken cancellation)
{
    var errors = ValidateMetadata(context.Metadata);

    if (errors.Count > 0)
    {
        return Fail(errors[0]);
    }

    var createResult = await _storage.Create(context, new CreateOptions()
    {

        Expiration = new AbsoluteExpiration(TimeSpan.FromMinutes(10)),
        Store = new TusDiskStore(Constants.FileDirectory),

    }, cancellation);

    _logger.LogInformation($"File created with id {createResult.FileId}");

    return CreateOk(createResult.FileId);
}

As the following which would make it easier to read and less complex to maintain:

 [Authorize(Policy = "create-file-policy")]
public override async Task<ITusCreateActionResult> Create(CreateContext context, CancellationToken cancellation)
{
    var errors = ValidateMetadata(context.Metadata);

    if (errors.Count > 0)
    {
        // Side note: This should probably allow for returning all the errors.
        return Fail(errors[0]);
    }

    var result = base.Create(context, cancellationToken);

    // NOTE: We could access the storage service here if needed by using
    // base.StorageService.Create or whatever method we wish to invoke.

    _logger.LogInformation($"File created with id {createResult.FileId}");

    return result;
}

I think this comment is interesting but I don't have all the details. Care to elaborate? :)
// TODO:
            // 1. Seperate request validation from file storage validation
            // 2. Validate request here
            // 3. Validate file storage in StorageService

From the above, we also need to re-add validation of the incoming request (but you already knew this :) )
Also overall this code needs to be cleaned up, but this is obviously a later issue since it's still a POC.

Also a thought: Should we integrate with response formatting in ASP.NET Core to allow other responses than simple text/plain? 🤔

Another idea based on the above: Could we maybe not use our own controller at all but rather integrate the storage service, intent etc into a regular controller as injected services? If so we would not need to implement all this custom code at all.

Given how much traction "Minimal APIs" in .NET6 is gaining I think that we should pause this feature to not do to much job before we know how Minimal APIs are received by the community. If it turns out that Minimal APIs is the new black and MVC is "legacy" it seems odd to rebuild the entire code base to something that is legacy.
https://gist.github.com/davidfowl/ff1addd02d239d2d26f4648a06158727

First of all, thanks for your feedback :)
I agreed with many of your suggestions and implemented them in my last commits to the POC
Addressing your comments

In the above setup the TusStorageService always contains the store (storage proxy probably) to use so there is no need for the endpoint to care. Compare this to how you would setup a EF context or a Redis connection. This would also solve the TODO in public virtual Task GetCapabilities().

Agreed. I implemented this in my last commits

Since the TusStorageService is "complete" there is no need for the [TusInheritCapabilities(typeof(TusDiskStore))] attribute.

Agreed and removed :)

I think that the TusEnableExtensionAttribute and TusDisableExtensionAttribute are a good idea but not implemented correctly. These will only not announce the functionality but the functionality will still be there.

Being able to announce extensions independently from TusStore was the reason why i intruduced this feature in the first place. For example, if you want to disable the termination extension but want to keep using TusDiskStore, how would you do it?
Just failing all termination requests while still announcing termination extension doesnt really seem like a good idea to me.

I'm not really sure what the usage is for the [TusController] attribute? It seems redundant as the controller must inherit from TusControllerBase anyway. We could probably rename TusControllerBase to just TusController.

The intended usage of [TusController] attribute is for assembly scanning to be able to tell base-classes and implementations apart and for users to disable a controller by removing the attribute.
For example if a user creates his own abstract class from TusControllerBase, assembly scanning is able to detect and ignore it, because the [TusController] attribute is missing.
Btw i named it TusControllerBase to match the naming convention of MVC's ControllerBase, but i dont mind to change that, if you dont like it.

Speaking of TusController. I'm not a fan of the abstract methods forcing users to implement each and every method to use this pattern. I can imagine that some users might want to just override a specific method, e.g. they want to add the file id and the current user id to a DB table and thus just wants to override the Create method.

I completely agree :)
I've made the TusController methods virtual in my latest commits

Given the above, we could probably change how we inject the StorageService to be the same as e.g. HttpContext (property injection).

Agreed

I think this comment is interesting but I don't have all the details. Care to elaborate? :)
// TODO:
// 1. Seperate request validation from file storage validation
// 2. Validate request here
// 3. Validate file storage in StorageService

The idea is to seperate validation into two parts.

One for checking if the request is not malformed (done by IntentHanlers)
One for checking if the operation itself to the storage is valid (done by StorageService)

Here is an example:
The Requirement new UploadOffset() checks if a request is malformed, therefore it should be executed in IntentHandler.
The Requirement new FileExist() checks if a storage operation is invalid, therefore it should be executed in StorageClient.
This has following advantages:

The IntentHandlers no longer depend on ITusStore
If needed, the user can handle invalid storage operations in the controller, by catching its exceptions

Also a thought: Should we integrate with response formatting in ASP.NET Core to allow other responses than simple text/plain? 🤔

I think it could indeed be useful for some users who want to return some json when the upload is finshed for example. I think we can discuss it at a later time

Another idea based on the above: Could we maybe not use our own controller at all but rather integrate the storage service, intent etc into a regular controller as injected services? If so we would not need to implement all this custom code at all.

I think it would be pretty difficult tu ensure it is compliant with tus spec. For example ho can we prevent an user from returning the wrong IActionResult or ensure all headers have been set correctly?
Also, a regular controller has a 1:1 request to action mapping, which makes it difficult to implement CreationWithUpload or Concatenation.

Given how much traction "Minimal APIs" in .NET6 is gaining I think that we should pause this feature to not do to much job before we know how Minimal APIs are received by the community. If it turns out that Minimal APIs is the new black and MVC is "legacy" it seems odd to rebuild the entire code base to something that is legacy.

I dont see minimal apis as an replacement for controllers, but rather as an addition to simplify the creation of small applications.
Also this poc already kind of supports minimal apis:
var builder = WebApplication.CreateBuilder(args);

builder.Services
    .AddTus()
    .AddStorage("my-storage", new TusDiskStore(@"C:\tusfiles\"), true)
    .AddEndpointServices();

var app = builder.Build();

app.MapTus("/files");

app.Run();

Note: In one of the next sections i explain in depth why i think that the controller pattern is still a good idea.
Current design
Configuration
services
    .AddTus()
    .AddController<MyTusController>()
    .AddStorage("my-storage", new TusDiskStore(@"C:\tusfiles\"), isDefault: true)
    .AddEndpointServices();

Storage
I decided to implement a kind of "named clients" pattern to access storage
To register your storage:
services.AddStorage("my-storage", new TusDiskStore(@"C:\tusfiles\"), isDefault: true)

The isDefault: true means, that evey tus controller uses this storage by default, if nothing else is configured.
Thats it, now you can use it in a controller:
[TusController]
[TusStorageProfile("my-storage")] // to change the used storage
public class MyTusController : TusControllerBase
{
    public override async Task<ICompletedResult> FileCompleted(FileCompletedContext context)
    {
        // StorageClient has been automatically injected
        await StorageClient.Delete(context.FileId);

        return BadRequest("Upload failed successfully. Please try again :)");
    }
}

To access your storage in DI services, inject ITusStorageClientProvider and then do:
var storageClient = await _storageClientProvider.Get("my-storage");
// or
var storageClient = await _storageClientProvider.Default();

Controllers
Registering a controller is still as simple as doing
endpoints.MapTusController<MyTusController>("/files");

You can configure more options by doing:
endpoints.MapTusController<MyTusController>("/controller/files", options =>
{
    options.StorageProfile = "my-storage";
    options.MaxAllowedUploadSizeInBytes = 1_000_000_000;
    options.MetadataParsingStrategy = MetadataParsingStrategy.AllowEmptyValues;
});

Alternatively you can configure the options using attributes:
[TusStorageProfile("my-storage")]
[TusMaxUploadSize(1_000_000_000)]
[TusMetadataParsing(MetadataParsingStrategy.AllowEmptyValues)]
public class MyTusController : TusControllerBase

Controller alternative
If users dont want to write a controller, they can use this simpler abstraction: (which uses EventsBasedTusController)
// uses the EventsBasedTusController under the hood
endpoints.MapTus("/files", (options) =>
{
    options.StorageProfile = "my-storage";
    options.Expiration = new AbsoluteExpiration(TimeSpan.FromMinutes(Constants.FileExpirationInMinutes));
    options.MetadataParsingStrategy = MetadataParsingStrategy.AllowEmptyValues;
    options.Events = new Events
    {
        OnFileCompleteAsync = ctx =>
        {
             logger.LogInformation($"Upload of {ctx.FileId} completed using {ctx.Store.GetType().FullName}");
             return Task.CompletedTask;
        }
    };
});

Examples of tus controllers
Simplest Tus Controller
[TusController]
public class VerySimpleTusController : TusControllerBase
{
    public override async Task<ICompletedResult> FileCompleted(FileCompletedContext context)
    {
        var tusFile = await StorageClient.Get(context.FileId);

        DoSomeProcessing(tusFile);

        return FileCompletedOk();
    }
}

Using database in a tus controller
[TusController]
public class DatabaseTusController : TusControllerBase
{
    private readonly ApplicationDbContext _dbContext;

    public SimpleDatabaseTusController(ApplicationDbContext dbContext)
    {
        _dbContext = dbContext;
    }

    public override async Task<ICreateResult> Create(CreateContext context)
    {
        var result = await StorageClient.Create(context);

        _dbContext.Insert(result.FileId);

        return CreateOk(result);
    }

    public override async Task<IDeleteResult> Delete(DeleteContext context)
    {
        await StorageClient.Delete(context.FileId);

        _dbContext.Delete(context.FileId);

        return DeleteOk();
    }
}

Why controller pattern?
In this section i want to explain why i advocate for a controller pattern and change the current design.
This may sound overly negative about the current design which is not my intent :)
I created the following diagram that will help me illustrate what i mean.
(correct me if i'm wrong)

I see the following problems with this design:

Intent Handlers have both protocol logic and storage logic (for example file expiration)
This should be seperated to increase modularity.

User Application Logic is not part of the Request Flow (left to right). Instead, it is a outside observer, only communicating through Events, Callbacks and special classes like IFileIdProvider.
This adds unneccessary complexity. Making User Application Logic a part of the Request Flow will fix this while giving the user code additional control.

One big options class for everything, regardless of the scope, is subobtimal.
Every scope should have their own options. Options should be configured once and not depend on some callback.

Everything uses ITusStore
In many classes there is a dependency on TusStore, that isnt really needed. Validation, Configuration and even the Intent Handlers and Protocol Handlers depend on TusStore.

Here is how the new design solves this:
Here is the diagram of the tus controller architecture used in my commit:

User code is now part of the request flow (left to right)
Removes the unnesecary need for events. Allows the users to intercept requests and change default behaiviur easily, without having to rewrite TusStore code and open unneccesary issues. Gives user appplication code more power

Options classes have been split up and moved to different scopes
Removes the unnesecary need for option callbacks and simplifies configuration

IntentHandlers, Protocol Handler and Request Validation do not depend on TusStore anymore
Increases Modularity and users can now implement new features using TusController without needing changes to the TusDiskStore

Seperation of concerns
IntentHandlers+Validation have been seperated from storage logic:

IntentHandlers now only handle protocol logic and do request validation
StorageService now handles storage logic and does storage validation

(Note: In my commit, IntentHandlers are named RequestHanlders and StorageService is named StorageClient)
What are your thoughts about this?

Thanks for the huge amount of work you have put into this. I'm very impressed and humbled by the amount of time and effort you put in <3 I have been swamped with my day job for a while now so activity on my end have been low. I will try to take some this week an check this out!

This was way more new code than expected :D I have started looking into it but it takes some time as I'm still recovering from a man cold. Let me know if I misunderstood something. I think these discussions are very good and increases both our understanding of the project and the different use cases. I will start by answering your comments and then move on to my new comments on the updated code.

Being able to announce extensions independently from TusStore was the reason why i intruduced this feature in the first place.

Good catch! This has been requested in other issues and will be implemented in the current design too.

The intended usage of [TusController] attribute is for assembly scanning to be able to tell base-classes and implementations apart and for users to disable a controller by removing the attribute.

Understood! I'm not really a fan of assembly scanning and think it adds extra complexity that is not really needed. Most apps running tusdotnet will not have that many controllers and source generators will soon take over the world for DI anyway (famous last words, don't quote me on this :) )

The IntentHandlers no longer depend on ITusStore
If needed, the user can handle invalid storage operations in the controller, by catching its exceptions

I don't really see this as good things to be honest. "No longer depend on ITusStore" is a side effect of this design and not a feature in itself. I can both see the benefit and the disadvantage of the second point. The current design garantuees that the request is OK (both in terms of parsing and storage) before any user code is executed so you know that everything is fine when your code runs. In the new design, this isn't the case and thus you must handle things yourself. For instance you might wish to save something to a database when a file is written but you cannot do this unless you first call the storage client as you have no idea if the request is OK or not yet. Might be an OK tradeoff, I'm just pointing it out.

I think it could indeed be useful for some users who want to return some json when the upload is finshed for example. I think we can discuss it at a later time

Agreed, this could be added at a later stage.

I think it would be pretty difficult tu ensure it is compliant with tus spec. For example ho can we prevent an user from returning the wrong IActionResult or ensure all headers have been set correctly?
Also, a regular controller has a 1:1 request to action mapping, which makes it difficult to implement CreationWithUpload or Concatenation.

I agree with this as well. My concern here is that users might thing of the tus controller as a MVC controller and it won't work exactly the same which will confuse people. Might just need a rename to something else. Maybe "Module" or some other abstract concept?

I dont see minimal apis as an replacement for controllers, but rather as an addition to simplify the creation of small applications.

I find this is to be a common misconception in regards to minimal APIs. "Minimal" here does not relate to "Small apps" but rather "minimum amount of ceremonial boilerplate". Same thing applies here, I think the name "controller" causes some confusion as it's really not a controller in the MVC sense.

I decided to implement a kind of "named clients" pattern to access storage

I like this. Only change I would like here is to not have a bool provided to the method but rather have explicit methods, e.g. AddDefaultStorage and AddNamedStorage. Also adding multiple default storage implementation should result in an exception (really nitpicking here).

I created the following diagram that will help me illustrate what i mean.

I think the categorization used here makes the current design look way worse than it actually is (I know this is not your intent). The current request flow looks like this:

When a request arrives the configuration factory (app.UseTus(<configFactory>)) is called to determine if tusdotnet should do anything with this request and what options to use. The options (yes even the options, not only the storage) can change between requests depending on a large amount of things, e.g. who the user is, what tenant they belong to etc
The protocol handler takes the request and the configuration and tries to determine the intent of the request, e.g. "write file"
If an intent is found, the PH will perform common tasks such as locking, verifying common headers (e.g Tus-Version), run the Authorize callback and validate that the request is OK to be performed. Each intent handler knows what rules apply to it.
The intent handler is executed and will fire events/callbacks for the user to interact with

One thing to note here is that IFileIdProvider is not part of the tusdotnet flow but is a helper for TusDiskStore. There are multiple other stores implemented which writes files in other types of private stores (Azure Storage, AWS, FTP etc) which have their own settings.

This should be seperated to increase modularity.

Why do we need to increase modularity here? We are not going to reuse each of the components anywhere and I think that knowing that the request is OK before running user code has its own value as you as a dev can focus on your things instead of worrying about tus-related things.

This adds unneccessary complexity. Making User Application Logic a part of the Request Flow will fix this while giving the user code additional control.

Not entirely sure I agree with this. I would definitely give more control to the user code but will also force the user to know much more about the quirks of the protocol. Might not be a bad thing but when issues occur it is something that we can easily work around with the current design which is much harder in the new one.

Every scope should have their own options. Options should be configured once and not depend on some callback.

The options used depends on the user, tenant, file type or whatever the user wants it to be. In an app I can see how what you propose is a good baseline but in a library we must take into consideration every possible use case. As we cannot possibly know them all we must support a way of configuring options more dynamically. There are live cases now where one tenant might have a max size of X while another have a max size of Y for example.

In many classes there is a dependency on TusStore, that isnt really needed. Validation, Configuration and even the Intent Handlers and Protocol Handlers depend on TusStore.

The store is basically the lowest level of everything here and is needed for pretty much everything that has to do with the actual file. Not sure I really follow this argument.

Allows the users to intercept requests and change default behaiviur easily, without having to rewrite TusStore code and open unneccesary issues. Gives user appplication code more power

This can indeed be a good thing but also makes things way more complex. Should the user be allowed to break the protocol? Maybe.

Removes the unnesecary need for option callbacks and simplifies configuration

The need for option callbacks are not unnecessary as I pointed out above.

Increases Modularity and users can now implement new features using TusController without needing changes to the TusDiskStore

I don't really follow this argument. Can you give an example of a feature that one needs to modify the store to implement?
Ok so on to my comments :D
Just to recap and make the rest of my comments a bit more understandable I think this is what we want to achieve:

Use endpoint routing to allow us to use the built in URL parsing, authorization etc
Navigate away from events and call the appropriate methods ourselves to have a different workflow

The first bullet point I totally agree with and see no issues with. To be the devil's advocate I can see some issues with the second one, which might just be because I haven't really seen a use case where it is needed.
This in event based form:
OnBeforeCreateAsync = async ctx =>
{
    var metadataErrors = ValidateMetadata(ctx.Metadata);

    if (metadataErrors.Count > 0)
    {
        ctx.FailRequest(metadataErrors);
    }
},
OnCreateCompleteAsync = async ctx =>
{
    _logger.LogInformation($"File created");
},

Would be replaced by this:

public override async Task<ICreateResult> Create(CreateContext context)
{
    var errors = ValidateMetadata(context.Metadata);

    if (errors.Count > 0)
    {
        return BadRequest(errors);
    }

    var result = await base.Create(context);

    _logger.LogInformation($"File created");

    return result;
}

Not a huge difference if you ask me. Adding additional events for e.g. file writes would solve the same thing? DefaultTusConfiguration is built around being virtual which allows users to inherit it and thus use DI and other modern tools in the same way as the controller would.
In general I think this PR does to much and changes to many things. Adding support for the controller pattern is one thing, changing how request validation is performed is another beast. Don't get me wrong, I'm very impressed by your ambition and what you have created here but I think we should move in smaller steps. Ripping out core parts of the library is prone to bugs and unintended/unforseen behavior which will make maintenance harder. Overall I'm not yet completely sold on the idea that this pattern is better than the old one but I can certainly be convinced :)
I think the TusProtocolHandlerEndpointBased is well implemented, aside from a few bugs with how options are actually shared between all controllers (it's a POC after all so no worries). The StoreAdapter is really nice too. I like the split between Features and Extensions! Where you really lost me is at the storage client / TusControllerBase and the multiple different overloads to call which seem to do very similar things. For instance I'm not a fan of the how storage operations work as the are just to many parameters, overloads and "option" instances for it to make sense. Sorry if this come across as blunt but I'm a firm believer in having an API where you do one thing in one way.
I do not like the "attributes as configuration" approach as it locks the user into using this pattern and prohibits them from using the configuration in other places in their code. The current design allows you to just inject the DefaultTusConfiguration into any service, which is currently not possible in the new design. I would rather see something similar to named storage but with options.
Smaller issues that we can definitely fix later and not really big problems right now (it's a POC after all):

Locks are taken to late, after storage validation has been performed, which will cause timing issues and corrupted files
Configuration can probably be simplified more. E.g. MetadataParsingStrategy can probably be removed as most clients are now updated and .NET6 is the new LTS so we can probably just automatically default to using the pipereader if the store supports it etc.
There are loads and loads of object creations which causes GC pressure. We could probably optimize this a lot.
creation-with-upload only works for non-chunked transfers
checksum-trailer does not work right now as there is an exception thrown instead of the file chunk being reverted
Loads of exceptions thrown in different places. Might be OK but hard to grasp which ones are used where. Same with result types. This is probably just me not knowing enough of the code base.

Thanks for your feedback!

Oh, and i just finished this poc for the most part, so there wont be big monster commits in the future anymore :)

Hi @SnGmng! Hope you had a good holiday. Just wanted to let you know that in the work with tus2 (https://github.com/tus/tus-v2) I played around with using the controller pattern in my POC. You can find it here: https://github.com/tusdotnet/tusdotnet/blob/POC/tus2/Source/tusdotnet/Tus2/Tus2Endpoint.cs
I'm still not entirely convinced that this is the way to go but I can definitely see some advantages to it. See user controller here: https://github.com/tusdotnet/tusdotnet/blob/POC/tus2/Source/TestSites/AspNetCore_netcoreapp3.1_TestApp/MyController.cs

@SnGmng Sorry for ating you several times :D I have continued to work on tus2 and tried to re-implement your solution there to get a feel for it. I went a slightly separate path that I think would solve both our use cases. Example code looks like this (netcoreapp3.1):
services.AddTus2(options =>
            {
                options.AddDiskStorage("MyStorageProfile", @"C:\tusfiles");
                options.AddHandler<MyTusHandler>();
            });

endpoints.MapTus2<MyTusHandler>("/files-tus-2", new EndpointConfiguration("MyStorageProfile"));

AddDiskStorage is an extension method that under the hood runs AddStorage("MyStorageProfile", new Tus2DiskStore...) and AddUploadManager("MyStorageProfile", new UploadManagerDiskBased(...)). Both AddStorage and AddUploadManager supports using a lambda to create the object using the current request:
services.AddTus2(options =>
{
    options.AddStorage(httpContext => new Tus2DiskStore(...))
}

The "name" parameter is optional and is not needed. The simplest form of configuration is this:
services.AddTus2(options =>
            {
                options.AddDiskStorage(@"C:\tusfiles");
                options.AddHandler<MyTusHandler>();
            });

endpoints.MapTus2<MyTusHandler>("/files-tus-2");

The storage and upload manager can be configured separately and you can also (of course) register your dependencies outside the AddTus2 call.
services.AddTus2(options =>
            {
                options.AddStorage("MyStorageProfile", httpContext => new ...);
                options.AddUploadManager("MyUploadManager", httpContext => new ...)
                options.AddHandler<MyTusHandler>();
            });
services.AddScoped<ITus2Store, MyTus2Store>();

endpoints.MapTus2<MyTusHandler>("/files-tus-2", new EndpointConfiguration(storageConfigurationName: "MyStorageProfile", uploadManagerConfigurationName: "MyUploadManager"));

// This one will use MyTus2Store
endpoints.MapTus2<MyTusHandler>("/files-with-default-storage");

I went the route (pun intended) with having the configuration on each endpoint to allow re-use of handlers between endpoints.  The endpoint configuration can later be extended to contain expiration, max file size etc. I also, as you can see, renamed "controller" to "handler" to make it less confusing with MVC controllers.
What do you thing about this solution? Would it fit your use case? Entire code can be found here: https://github.com/tusdotnet/tusdotnet/tree/POC/tus2/Source/tusdotnet/Tus2

I'll try once again (last time promise). @SnGmng any progress on this PR/issue? :)

Oh no problem, you can "at" me as much as you want :)
I was very busy the last two months, sorry for the delay.
Answering your comments

My concern here is that users might thing of the tus controller as a MVC controller and it won't work exactly the same which will confuse people. Might just need a rename to something else. Maybe "Module" or some other abstract concept?

Haha i had to smile when i read this because it reminded me of Discord.NET :) This is exactly what Discord.NET has been doing! They also use kind of the same controller-like pattern and also named it "Module".
Also, I think Discord.NET is a great example of a framework which benefits from the controller pattern. I think we can learn a lot from that library.

Increases Modularity and users can now implement new features using TusController without needing changes to the TusDiskStore

I don't really follow this argument. Can you give an example of a feature that one needs to modify the store to implement?

I was thinking of things like file encryption, rate limiting, changing the metadata storage format, and any other modification of the writing process. Examples are at the end of this comment.

Not a huge difference if you ask me. Adding additional events for e.g. file writes would solve the same thing? DefaultTusConfiguration is built around being virtual which allows users to inherit it and thus use DI and other modern tools in the same way as the controller would.

Yes, i agree. But i think it is less about the functionality and more about ease of use and how intuitive the api is.
And configuring tus using a callback kinda defeats the purpose of configuration, IMO. I think configuration should be "configured once" and not be dynamically computed using a httpContext. When i first used tusdotnet, it felt kinda wrong having the application logic mixed with static configuration inside a lambda, which was one of the reasons i created the issue in the first place: to seperate application logic and configuration.
The controller pattern and the event based design are both roughly equivalent functionality wise, but i think the controller pattern provides a much more straightforward way of solving problems. Most users unfortuantely don't bother to read the wiki, so it is very important to have an design that is familiar and self-explainatory and i think the controller pattern is a good choice for that.

In general I think this PR does to much and changes to many things. Adding support for the controller pattern is one thing, changing how request validation is performed is another beast.

Yes i agree. I tried to change as little of the functionality as possible and just restructure the code, and i think i retained most of the default functionality for the most part.
There are a few exceptions where i changed the default behaivior: trailing-checksum and creation-with-upload (more on that later)

I think the TusProtocolHandlerEndpointBased is well implemented, aside from a few bugs with how options are actually shared between all controllers

Can you explain this further? I dont think that there is a bug that it shares the options, because this POC has passed all the 600 unit tests which all have different options. One core difference between the normal TusProtocolHandler and TusProtocolHandlerEndpointBased is that the former is created exacly once and the latter is created once per endpoint (and each controller has its own endpoint), so theoretically, each endpoint should have its own configuration. Correct me if i'm wrong.

The StoreAdapter is really nice too. I like the split between Features and Extensions!

Thats nice to hear! What do you think about StoreChecksumAlgorythms? At first I thought it was pretty unneccessary bu as i implemented checksum-trailers, it was pretty handy because i didnt have to keep track of caching the result of GetChecksumsAsync, and it acrtually reduced the calls to GetChecksumsAsync inside the unit tests, which i had to adjust because of this.

Where you really lost me is at the storage client / TusControllerBase and the multiple different overloads to call which seem to do very similar things.

Yeah these classes still need to be cleaned up. About the differnt overloads of the storage client, some of them where simplifing usage inside a controller, and i already moved them to extension classes in my latest commit.

For instance I'm not a fan of the how storage operations work as the are just to many parameters, overloads and "option" instances for it to make sense. Sorry if this come across as blunt but I'm a firm believer in having an API where you do one thing in one way.

Agreed! The "storage operation" classes where just a temporary soultion to reduce the size of the storage client class. I focussed only on the user facing api, so internal classes may look a bit messy, sorry :) Its obvious that so many method parameters is not good code and this part of the poc definetly needs some work.
The thing about the store dependency...

I don't really see this as good things to be honest. "No longer depend on ITusStore" is a side effect of this design and not a feature in itself. I can both see the benefit and the disadvantage of the second point. The current design garantuees that the request is OK (both in terms of parsing and storage) before any user code is executed so you know that everything is fine when your code runs. In the new design, this isn't the case and thus you must handle things yourself. For instance you might wish to save something to a database when a file is written but you cannot do this unless you first call the storage client as you have no idea if the request is OK or not yet. Might be an OK tradeoff, I'm just pointing it out.

Thanks for pointing that out! I didnt really think of this before. The point of moving validation to TusStorageService was to make it reusable and replaceable to suit following groups of people:

People that use TusStorageService but only in the Controller
People that use TusStorageService in the Controller and want to reuse it in other parts of the app
People that use their own storage service/backend in the Controller, replacing the TusStorageService

Here is the problem:

If we place validation inside the IntentHandler,

group 2 has to rewrite validation code in all the places they reuse TusStorageService
group 3 is forced to write an ITusStore as an adapter to their own storage service because of the ITusStore dependency

If we place validation inside the TusStorageService, users cant know if the request is valid until TusStorageService has been called

Suggested solution:
I would opt for a kind of hybrid solution that does the validation inside IntentHandler when possible by default but also inside TusStorageService if it is used outside the controller. This is not a very beautiful solution, so if you have a better one I'll be happy to hear it!

Why do we need to increase modularity here? We are not going to reuse each of the components anywhere and I think that knowing that the request is OK before running user code has its own value as you as a dev can focus on your things instead of worrying about tus-related things.

I disagree. For example, I need to reuse my storage backed code accross multiple upload methods, including TUS, FTP, etc. and the modular design is very useful for me.

With the new design, i can just inject my custom services, used for storing the file, directly into the controller and call them directly.
In the current design i would have to write an ITusStore adapter just for TUS and if i want to call ITusStore in other places, i would additionaly be forced to, again, add custom validation on top of it, adding unneccesary complexity.

In my application, the user must be able to upload his files using multiple common methods:

Using TUS (obviously :))
Using simple http POST request
Using FTP
Using remote url upload

All these methods should use a common implementation of a custom "storage service", which takes care of storing the data and does fancy things like geo-replication and erasure-coding.
The modular design of this POC would be very helpful for my use case.

Just to recap and make the rest of my comments a bit more understandable I think this is what we want to achieve:

Use endpoint routing to allow us to use the built in URL parsing, authorization etc
Navigate away from events and call the appropriate methods ourselves to have a different workflow

The first bullet point I totally agree with and see no issues with. To be the devil's advocate I can see some issues with the second one, which might just be because I haven't really seen a use case where it is needed.

I would put it this way:

Use endpoint routing to allow us to use the built in URL parsing, authorization etc
Allow users to intercept the request flow
Decouple the tus protocol and file storage from each other
Move away from the option callback to have a different workflow

creation-with-upload only works for non-chunked transfers

Oh good catch, i missed that! Note that this POC passes all unit tests, meaning that there is no unit test which tests this behaivior. It would be a good decision to write a unit test for that.

checksum-trailer does not work right now as there is an exception thrown instead of the file chunk being reverted

What makes you think that? The file chunk is reverted here, which is called from here
Note that this POC passes all unit tests. If you think that the behaivior of this POC is invalid, then there is a missing unit test for it. This would actually be an interesting aproach to improve unit test coverage: Try to make my POC fail by writing more unit tests for this library! :)
Examples of how the controller pattern solves many problems better
I've compiled a bunch of issues that the controller pattern solves better, imo.
Add option to limit the upload speed #78
Old solution:
app.Use((ctx, next) =>
{
    ctx.Request.Body = new ThrottledStream(ctx.Request.Body, maximumBytesPerSecond: 512000);

    return next.Invoke();
});

app.UseTus(...);

New solution:
public override Task<IWriteResult> Write(WriteContext context)
{
    context.RequestStream = new ThrottledStream(context.RequestStream, maximumBytesPerSecond: 512000);
    return base.Write(context);
}

How to implement file encryption? #130
Old solution:
app.Use((ctx, next) =>
{
    ctx.Request.Body = new EncryptedStream(ctx.Request.Body);
    return next.Invoke();
});

app.UseTus(...);

New solution:
public override Task<IWriteResult> Write(WriteContext context)
{
    context.RequestStream = new EncryptedStream(context.RequestStream);
    return base.Write(context);
}

Access TusStore from Controller #125
Old solution:
OnFileCompletedAsync = eventContext =>
{
    using (var serviceScope = app.ApplicationServices.CreateScope())
    {
        serviceScope.ServiceProvider.GetRequiredService<CmsDbContext>();
        serviceScope.ServiceProvider.GetRequiredService<IVideoManager>();

        var fms = serviceScope.ServiceProvider.GetRequiredService<IFileManagementService>();
        var file = await eventContext.GetFileAsync();

        await fms.HandleFileUploadFinished(file, httpContext.RequestAborted);
    }
}

New solution:
public class MyTusController
{
    private readonly IFileManagementService _fms;

    public MyTusController(IFileManagementService fms)
    {
        _fms = fms;
    }

    public override async Task<ISimpleResult> FileCompleted(FileCompletedContext context)
    {
        var file = await StorageClient.Get(context.FileId, HttpContext.RequestAborted);
        await fms.HandleFileUploadFinished(file, HttpContext.RequestAborted);

        return Ok();
    }
}

Use tus as a service with own controller and endpoint #117
Old solution:
app.UseTus(async httpContext =>
{
    if(!httpContext.User.Identity.IsAuthenticated)
        return null; // Disables tusdotnet for this request.

    string userStorageLocation = await GetUserStorageLocationFromDatabase(httpContext.User);

    return new DefaultTusConfiguration
    {
        Store = new TusDiskStore(userStorageLocation),
        UrlPath = "/files",
        Events = new Events
        {
            OnFileCompleteAsync = async eventContext =>
            {
                ITusFile file = await eventContext.GetFileAsync();
                await DoSomeProcessing(file);
            }
        }
    }
});

New solution:
public class MyTusController
{
    private readonly IUserService _userService;
    private readonly IUserService _dbContext;

    public MyTusController(IUserService userService, IDbContext dbContext)
    {
        _userService = userService;
        _dbContext = dbContext;
    }

    public override async Task<ICreateResult> Create(CreateContext context)
    {
        if(!this.User.Identity.IsAuthenticated)
        {
             return BadRequest();
        }

        var user = _userService.GetUser(this.User);
        if (!user.CanUpload)
        {
             return BadRequest();
        }

        var myStore = new TusDiskStore(user.StorageLocation);
        var storageClient = TusStorageClient.CreateNew(myStore);

        var result = await storageClient.Create(context);

        _dbContext.Files.Insert(result.FileId, ...)

        return CreateStatus(result);
    }

    public override async Task<ISimpleResult> FileCompleted(FileCompletedContext context)
    {
        // ...

        return Ok();
    }
}

I think in general the "new solution" looks much cleaner especially in the last one, but that may be personal bias. What do you think of it?

Oh and i'll need some more time to look at your tus2 implementation and give feedback, but as far as i can tell, it definetly already looks promising!

Oh no problem, you can "at" me as much as you want :)
I was very busy the last two months, sorry for the delay.

No worries at all about the delay. I just wanted to check if the issue was still active or dead. :) After implementing the code for the tus2 POC I think most of my concerns and previous comments in this thread is gone :D I've spent a lot of time in the last couple of months with the tus group working on the tus2 spec (and the POC) and the handler/controller/module pattern (I'll call it handler below due to the tus2 POC using that name) is indeed more intuitive and separating concerns more is just a win for all of us. I'll still answer your comments below, if nothing else for the sake of argument ;)

Haha i had to smile when i read this because it reminded me of Discord.NET :) This is exactly what Discord.NET has been doing! They also use kind of the same controller-like pattern and also named it "Module".
Also, I think Discord.NET is a great example of a framework which benefits from the controller pattern. I think we can learn a lot from that library.

Thanks for the link! I'll have a look and draw some inspiration.

I was thinking of things like file encryption, rate limiting, changing the metadata storage format, and any other modification of the writing process. Examples are at the end of this comment.

Fair point and good examples. These seem much easier to follow in this pattern than in the previous one (even if that isn't that hard to follow either).

Yes, i agree. But i think it is less about the functionality and more about ease of use and how intuitive the api is.
And configuring tus using a callback kinda defeats the purpose of configuration, IMO. I think configuration should be "configured once" and not be dynamically computed using a httpContext. When i first used tusdotnet, it felt kinda wrong having the application logic mixed with static configuration inside a lambda, which was one of the reasons i created the issue in the first place: to seperate application logic and configuration.

I can see that, it's a fair point. In the tus2 POC I added the functionality to both add a single configuration object or have a factory similar to how it's done in tus1. I think this was a good compromise as I know a lot of people are using the factory to create configs on the fly per user.

Most users unfortuantely don't bother to read the wiki, so it is very important to have an design that is familiar and self-explainatory and i think the controller pattern is a good choice for that.

100% agree!

Can you explain this further? I dont think that there is a bug that it shares the options, because this POC has passed all the 600 unit tests which all have different options. One core difference between the normal TusProtocolHandler and TusProtocolHandlerEndpointBased is that the former is created exacly once and the latter is created once per endpoint (and each controller has its own endpoint), so theoretically, each endpoint should have its own configuration. Correct me if i'm wrong.

This was probably an oversight on my end, sorry about that. Can't remember the issue and I can't seem to recreate the bug either so I was probably just overtired or something. Sorry for causing confusion!

Thats nice to hear! What do you think about StoreChecksumAlgorythms? At first I thought it was pretty unneccessary bu as i implemented checksum-trailers, it was pretty handy because i didnt have to keep track of caching the result of GetChecksumsAsync, and it acrtually reduced the calls to GetChecksumsAsync inside the unit tests, which i had to adjust because of this.

I hade the same first thought but I think it plays out pretty good if we can reduce the number of calls to the actual store. Can't see it happening that the store would return different algorithms on different calls. Not impossible but very unlikely.
Side note here: I have been thinking about adding some kind of "request cache" for values that should not change during a request (such as checksum algorithms). Maybe also having it updatable so that e.g. Upload-Offset could always be read from a single source and not needed to be fetched from the store each time. This isn't really a problem for the disk store but other stores might suffer from poor performance due to network calls etc. This is probably a huge change though to get it correct so it's just on the drawing board so far.

Yeah these classes still need to be cleaned up. About the differnt overloads of the storage client, some of them where simplifing usage inside a controller, and i already moved them to extension classes in my latest commit.

Nice! 👍

Agreed! The "storage operation" classes where just a temporary soultion to reduce the size of the storage client class. I focussed only on the user facing api, so internal classes may look a bit messy, sorry :) Its obvious that so many method parameters is not good code and this part of the poc definetly needs some work.

Glad to hear it :)

Thanks for pointing that out! I didnt really think of this before. The point of moving validation to TusStorageService was to make it reusable and replaceable to suit following groups of people:

Funny you should mention this as I had similar thoughts implementing the tus2 POC :D I didn't implement it this way as it's only a POC for the new protocol but I had an idea that one might want to split the validation of the request to its own interface, say ITusValidator. This implementation could then be run anywhere (or be skipped if so). The handler invoker (TusProtocolHandlerEndpointBased in your code) could just grab the validator and run the appropriate method on it depending on what intent is performed by the client. This brings me to my other thought: If you look at most of the "OnBeforeX" events they will most often look like something like this:
OnBeforeCreateAsync = ctx =>
                    {
                        // Partial files are not complete so we do not need to validate
                        // the metadata in our example.
                        if (ctx.FileConcatenation is FileConcatPartial)
                        {
                            return Task.CompletedTask;
                        }

                        if (!ctx.Metadata.ContainsKey("name") || ctx.Metadata["name"].HasEmptyValue)
                        {
                            ctx.FailRequest("name metadata must be specified. ");
                        }

                        if (!ctx.Metadata.ContainsKey("contentType") || ctx.Metadata["contentType"].HasEmptyValue)
                        {
                            ctx.FailRequest("contentType metadata must be specified. ");
                        }

                        return Task.CompletedTask;
                    },

The above is just simple validation of the request so why not allow the dev to add their own (and possibly remove built in validation) from the request pipe? This would also be doable with a separate validator interface.

I disagree. For example, I need to reuse my storage backed code accross multiple upload methods, including TUS, FTP, etc. and the modular design is very useful for me.

This is embarassing on my end that I did not think of that. 🙈 Thanks for the clarification!

With the new design, i can just inject my custom services, used for storing the file, directly into the controller and call them directly.
In the current design i would have to write an ITusStore adapter just for TUS and if i want to call ITusStore in other places, i would additionaly be > forced to, again, add custom validation on top of it, adding unneccesary complexity.

100% agree with this.

I would put it this way:

Use endpoint routing to allow us to use the built in URL parsing, authorization etc
Allow users to intercept the request flow
Decouple the tus protocol and file storage from each other
Move away from the option callback to have a different workflow
creation-with-upload only works for non-chunked transfers

100% agree again :D

Oh good catch, i missed that! Note that this POC passes all unit tests, meaning that there is no unit test which tests this behaivior. It would be a good decision to write a unit test for that.

Agree with this aswell. Unfortunately the unit testing framework in .NET is really hard to use with "real" http requests as they are all abstractions on abstractions on... . If you look at e.g. Checksum_Is_Verified_If_Client_Disconnects_Using_Trailing_Header you can see what I mean. It's not really sending a trailing header, it just pretends that it did and hopefully Microsoft's code for handling that is not broken :) The first "unit test" of checksum-trailer was really just a simple node script running requests. 🤷♂

What makes you think that? The file chunk is reverted here, which is called from here
Note that this POC passes all unit tests. If you think that the behaivior of this POC is invalid, then there is a missing unit test for it. This would > actually be an interesting aproach to improve unit test coverage: Try to make my POC fail by writing more unit tests for this library! :)

I think I hit this line when I disconnected the client and thus though it would throw an exception. Sorry about that :) After six or so years building this project I can most certainly say that detecting client disconnection is incredibly hard and fragile. I have done some fixes in the latest master so probably just and old bug on my end.

Examples of how the controller pattern solves many problems better

Great examples! All of them are much clearer and easier to use than the current solutions.

Oh and i'll need some more time to look at your tus2 implementation and give feedback, but as far as i can tell, it definetly already looks promising!

Please do! I have borrowed (read stolen ;) ) lots of your ideas from this issue and others. It uses the handler pattern, endpoint routing, pipereaders and all the new fancy features. The tus2 protocol is simpler and there are currently no extensions which make everything a bit easier.There are some discussions in this issue. Feel free to tag along!

It's been a long time for this issue but work is finally underway. Since #196 changed how the core functionality works I have abandoned the POC and started working on a new branch here: dev/148-handler-pattern. It still requires some polishing but overall it works and all tests pass.
Still some work needed though:

Configuration has not been set yet. The idea is to keep it really simple and just inject a single ITusStore (and whatever else is needed) into the IoC container. For more advanced use cases there is the Initialize and Finalize methods on the handler that can be used similarly to the configuration factory when running MapTus/UseTus.
More tests are needed for handler specifics (e.g. calling UploadCompleted and similar)
... probably more that I can't think of right now :D

Great that work is underway! I am impressed by your continued efforts regarding this issue. It is quite rare to see.
I have looked at your code yesterday, but i have quite a few objections about the architecture.
How i would design the library
In the past there was quite a heavy focus on the handler/controller class, but it is not the handler class itself that is important, it is the architectual design decisions that surround it.
The architecture i proposed in the past was to split up the library into multiple layers, which should have no knowledge about the logic in the layer above and work independently:

Protocol Layer (in my POC: RequestHandler)
Application Code Layer (in my POC: TusController)
Service Layer (in my POC: TusStorageClient)
Repository Layer (ITusStore)

Q: Why have a seperate protocol layer?

People (including me) may only want to leverage the tus protocol without being forced to use any predefined storage backend (more on that later)
You should be able to use the tus protocol without knowledge about the storage backend

Q: Why have a seperate service layer (that is protocol agnostic and does not depend on the http context)?

People may want to reuse the storage backend for other upload methods (like FTP, simple HTTP, or remote uploads)
You should be able to use the storage backend without knowledge about the tus protocol

The problems i see with the current approach
Now, the problem i see, is, it seems like dev/148-handler-pattern does not split up the concerns in any way. It seems like any class can modify anything.
The service layer and protocol layer are mixed all over the place:

TusV1ProcessRunner seems to modify both the request and invoke ITusStore?
Tus Protocol logic seems to run both upstream and downstream of TusHandler code?

The knowledge of the http request shoud stop right at the handler. TusV1ProcessRunner should have no access to the request, and only the ITusStore.
It is quite important to me that i can implement the tus handler actions without being forced to use any predefined storage backend, and i can just use my own injected services o accomplish that.
I am working on a video streaming CDN and it has quite a complex storage backend if you can imagine :)
When i first started using tusdotnet, i ran in to a few problems. Tusdotnet provided no easy way to intercept/change the request flow and quite a heavy coupling to ITusStore. It was the reason why i was motivated to make a proposal
What i would change
TusV1ProcessRunner
TusV1ProcessRunner is acting as a bridge class between the TusHandler (application code) and ITusStore (storage repository). This is basically the same thing that a service is supposed to do, so i would assume it is the service layer and probably rename it to something like "TusV1StorageService" or "TusV1StorageClient" and I would propose following modifications to it:

Remove any references to the HTTP request or context in TusV1ProcessRunner and instead pass in the necessary information as parameters. This will make it more modular and easier to reuse in other contexts.
Refactor TusV1ProcessRunner to only interact with the storage backend and not handle any tus protocol logic. This should be handled in the separate protocol layer.
Consider implementing a way for users to easily inject their own storage backend implementation, rather than being limited to the predefined ITusStore and TusV1ProcessRunner. This could be done through a dependency injection framework or by providing a simple way to register custom implementations.

TusV1HandlerRunner and IntentHandlers
These classes are basically the protocol layer. I would propose following modifications to it:

In the IntentHandlers, instead of having the logic for handling the tus protocol mixed with the logic for interacting with the storage backend, I would propose separating these concerns. The IntentHandlers should only be responsible for handling the tus protocol logic and not interacting with the storage backend directly. Instead, it should call the application code (TusHandler) which then interacts with the storage backend using the TusV1StorageService (aka: TusV1ProcessRunner) to handle these tasks.
To achieve this, the TusV1HandlerRunner should only have access to the HTTP request and should not be able to use ITusStore or any other storage backend related classes in any way. It should only interact with the storage backend through TusHandler.

Thats it for now, Let me know what you think about all this :)

Thanks for the feedback. As I've said before I'm not going to change the architecture and thus rewrite the whole thing from scratch. I'll look into the rest of your comments the next time I have some spare time to work on this.

@smatsson Any update on this?

@promontis Sorry no I've been busy with other things, amongst them the new Resumable Uploads For Http (RUFH) spec and implementation.
I have been meaning to close this issue as I don't see this happening. I don't think the proposed architecture is any better than what is currently in place and there haven't been any input from the community in over 6 months. I would much rather put my time and effort into RUFH or the new concept of using the built in ASP.NET Core model binding.


Add tournament play fuzz test back in

I was trying to reproduce #59, but I'm not seeing it anymore. It may have been eliminated from all of the fixups with folder -> active/still_playing changes.

bors r+


Support role based access control w/JWT's

- Do you want to request a feature or report a bug?
feature
- What is the current behavior?
it is impossible to test JWT site behavior using netlify-dev
- If the current behavior is a bug, please provide the steps to reproduce.
Set up redirects with a Role=x condition and observe that things do not work as expected.
- What is the expected behavior?
Redirects with a Role=x specification should work as expected.
- Please mention your node.js, and operating system version.
all versions.

I'm going to close this one in favor of #529 because that issue has more information.


Configure the test db to rollback after each test

Currently BE tests save data to the test db, which should not be happening

Using Jest you might be able to use a beforeEach to reset your db before every test.