Feat: Use tika to detect content type more accurately

[topikachu]

Use tika to detect content type more accurately.
I perform a manual test that the content type in s3 is expected.

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[topikachu]

@jglick
Could you take a look at this change?

[jglick]

652K (no transitive deps) so will slow down

artifact-manager-s3-plugin/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java

Line 123 in 71934b1

Map<String, String> contentTypes = workspace.act(new ContentTypeGuesser(new ArrayList<>(artifacts.keySet()), listener));

a bit when first called on a given agent (typically every build, when using one-shot Clouds). Not sure it is worth the cost to get more correct content types in some cases; would prefer for Files.probeContentType to be improved in the JDK. Did you have a particular content type that the plugin currently fails to detect properly?

[topikachu]

652K (no transitive deps) so will slow down

artifact-manager-s3-plugin/src/main/java/io/jenkins/plugins/artifact_manager_jclouds/JCloudsArtifactManager.java

Line 123 in 71934b1

Map<String, String> contentTypes = workspace.act(new ContentTypeGuesser(new ArrayList<>(artifacts.keySet()), listener));

a bit when first called on a given agent (typically every build, when using one-shot Clouds). Not sure it is worth the cost to get more correct content types in some cases; would prefer for Files.probeContentType to be improved in the JDK. Did you have a particular content type that the plugin currently fails to detect properly?

My ".log" file and ".json" files are all in "binary/octet-stream" type.
So when I click the file, the browser pops up a download dialog.

I noticed that it's MasterToSlaveFileCallable so the classes are sent to the agent.
I'm not sure if Files.probeContentType can be improved because it's at the agent side and use the system-wide SPI.

One solution is to create another ContentTypeGuesser using tika implementation and make this an option on the UI side

What do you think about it?

[jglick]

Ouch, *.json seems like a pretty basic failure.

Prefer to avoid cluttering the UI with obscure options.

A possible compromise: adjust ContentTypeGuesser to first try the Java Platform’s built-in techniques, as now. If these fail (binary/octet-stream fallback), then and only then use Tika. Might need to include the static references to Tika in a distinct method, to make sure the JVM does not try to load that JAR unless and until the method is called. (It is possible to use the support-core plugin to verify this interactively. There is probably some way to verify this in a functional test too, but that sounds like overkill.)

[topikachu]

Ouch, *.json seems like a pretty basic failure.

Prefer to avoid cluttering the UI with obscure options.

A possible compromise: adjust ContentTypeGuesser to first try the Java Platform’s built-in techniques, as now. If these fail (binary/octet-stream fallback), then and only then use Tika. Might need to include the static references to Tika in a distinct method, to make sure the JVM does not try to load that JAR unless and until the method is called. (It is possible to use the support-core plugin to verify this interactively. There is probably some way to verify this in a functional test too, but that sounds like overkill.)

I change the code that only uses tika when JRE can't detect the content type.
I run the test NetworkTest with -Dorg.jvnet.hudson.test.HudsonTestCase.slaveDebugPort=5005 so I can debug the agent side.
The hudson.remoting.RemoteClassLoader only downloads the 'tika' jar until the method is called.

[jglick]

Nice! Would you mind adding something to

artifact-manager-s3-plugin/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/s3/JCloudsArtifactManagerTest.java

Lines 292 to 303 in 728e7db

	p.setDefinition(new CpsFlowDefinition("node('remote') {writeFile file: 'f.txt', text: '" + text + "'; writeFile file: 'f.html', text: '" + html + "'; writeFile file: 'f', text: '\\u0000'; archiveArtifacts 'f*'}", true));
	j.buildAndAssertSuccess(p);
	WebResponse response = j.createWebClient().goTo("job/p/1/artifact/f.txt", null).getWebResponse();
	assertThat(response.getContentAsString(), equalTo(text));
	assertThat(response.getContentType(), equalTo("text/plain"));
	response = j.createWebClient().goTo("job/p/1/artifact/f.html", null).getWebResponse();
	assertThat(response.getContentAsString(), equalTo(html));
	assertThat(response.getContentType(), equalTo("text/html"));
	response = j.createWebClient().goTo("job/p/1/artifact/f", null).getWebResponse();
	assertThat(response.getContentLength(), equalTo(1L));
	assertThat(response.getContentType(), containsString("/octet-stream"));

showing that *.json or whatever now gets detected properly?

[jglick]

(BTW for purposes of review it is nicer to push follow-on commits, as rebasing breaks incremental review. Does not matter much in this case since the diff is so short to begin with.)

[topikachu]

Nice! Would you mind adding something to

artifact-manager-s3-plugin/src/test/java/io/jenkins/plugins/artifact_manager_jclouds/s3/JCloudsArtifactManagerTest.java

Lines 292 to 303 in 728e7db

	p.setDefinition(new CpsFlowDefinition("node('remote') {writeFile file: 'f.txt', text: '" + text + "'; writeFile file: 'f.html', text: '" + html + "'; writeFile file: 'f', text: '\\u0000'; archiveArtifacts 'f*'}", true));
	j.buildAndAssertSuccess(p);
	WebResponse response = j.createWebClient().goTo("job/p/1/artifact/f.txt", null).getWebResponse();
	assertThat(response.getContentAsString(), equalTo(text));
	assertThat(response.getContentType(), equalTo("text/plain"));
	response = j.createWebClient().goTo("job/p/1/artifact/f.html", null).getWebResponse();
	assertThat(response.getContentAsString(), equalTo(html));
	assertThat(response.getContentType(), equalTo("text/html"));
	response = j.createWebClient().goTo("job/p/1/artifact/f", null).getWebResponse();
	assertThat(response.getContentLength(), equalTo(1L));
	assertThat(response.getContentType(), containsString("/octet-stream"));

showing that *.json or whatever now gets detected properly?

Done!

[jglick]

Looks nice, thanks!