Add public share results feature #20

smathews-techempower · 2020-01-22T00:08:58Z

This adds a new file store directory that is meant for storing results file uploads from anonymous users. It is limited to a configurable size and prevents uploads larger than a different configurable size. All uploads are given a new random filename, called a share ID.

There is an API endpoint that is meant to accept uploads from the benchmarks toolset upon run completion. Additionally, this adds a paste bin style page that can be used by anonymous users to upload or paste results using a UI. Results files are compressed to zip files for storage, making for significant storage saving.

After successful upload, the results file is accessible at /share-results/{shareId}.json.

Addressing #3 from issue #19

- add the new test_metadata field to results, optional because it was only recently included in results by the toolset - add a new directory for public results uploads - add handler to support upload and viewing of results json files - give each upload a new random filename, preventing overwrites

- add new handler allowing paste or upload of results json files - move the URLs to config - limit share upload file size and size of upload directory - centralize upload logic to a new service - add jdwp allowing remote debugging of java process locally using intellij

# Conflicts: # example-config.yml # src/main/java/tfb/status/bootstrap/ServicesBinder.java # src/main/java/tfb/status/config/ApplicationConfig.java # src/main/java/tfb/status/config/FileStoreConfig.java # src/main/java/tfb/status/service/ApplicationConfigFactory.java # src/test/java/tfb/status/handler/UploadResultsHandlerTest.java # src/test/java/tfb/status/service/ApplicationConfigFactoryTest.java

michaelhixson

The biggest thing missing from this PR is tests. Tackle the other comments first, then do add tests for every new handler, service, and utility class. There is a fairly good test infrastructure in place already and plenty of examples, but let me know if it's unclear how to test a particular feature.

I suspect we'll want to replace the existing src/test/resources/test_managed_files results files with ones that contain testMetadata to help with this. There is a ResultsTester class that can be used for dynamically generating new fake results (for a test to upload, for example) based on these files, and I'm guessing these tests will benefit from using ResultsTester.

michaelhixson · 2020-01-22T16:45:49Z

example-config.yml

@@ -30,6 +30,8 @@

 #fileStore:
 #  root: managed_files
+#  max_share_directory_size_bytes: 1000000000
+#  max_share_file_size_bytes: 5000000


Use lowerCamelCase for consistency with the other settings.

michaelhixson · 2020-01-22T16:48:54Z

docker-compose.yml

@@ -6,8 +6,10 @@ services:
      args:
        SKIP_TESTS: "true"
    ports:
-      - "127.0.0.1:80:80"
+      - "127.0.0.1:8080:80"


Was it necessary to change this? Personally I prefer to have HTTP servers listen on port 80.

I'll put it back to port 80, for me locally it fails to bind to port 80 due to some acces permissions issue. I changed it to 8080 to get it working quickly rather than look into it, and accidentally committed it.

michaelhixson · 2020-01-22T16:54:44Z

src/main/java/tfb/status/config/UrlsConfig.java

+  }
+
+  private static final String DEFAULT_TFB_STATUS = "http://localhost:8080";
+  private static final String DEFAULT_TE_WEB = "http://localhost:3000";


I don't think these are good default values. It would be bad for either one to make it into production, and neither one would be correct for me in local development. I'd say change them to the production URLs. Or, if the functionality that uses this can be disabled when these settings are absent (like EmailSender when EmailConfig is absent), then make these nullable. I haven't gotten far enough in the review to know if the nullable/disabled route is workable.

src/main/java/tfb/status/config/UrlsConfig.java

michaelhixson · 2020-01-22T17:04:30Z

src/main/java/tfb/status/config/UrlsConfig.java

+      @JsonProperty(value = "tfbStatus", required = true)
+      @Nullable String tfbStatus,
+
+      @JsonProperty(value = "teWeb", required = true)


These should be required = false.

michaelhixson · 2020-01-22T18:57:47Z

src/main/java/tfb/status/service/ShareResultsUploader.java

+   * @param in The byte channel containing the raw results JSON.
+   * @see #upload(Path)
+   */
+  public ShareResultsJsonView upload(ReadableByteChannel in)


Given other comments I made, I'm expecting this method to change to accept either an InputStream or a ByteSource.

michaelhixson · 2020-01-22T19:04:16Z

src/main/java/tfb/status/service/ShareResultsUploader.java

+
+  /**
+   * Upload the given byte channel containing the raw results JSON. Creates
+   * a temporary file from the bytes and passes it to {@link #upload(Path)}.


This project uses an 80 character margin. Here, there is room for "a" to fit on the previous line.

I'm not particularly concerned with this "a", but I notice a lot of cases like this. Are you eyeballing the margin? If so, I suggest using IntelliJ's "visual guides" feature. Editor - Code Style - General - Visual guides. I have mine set to "80, 100, 120" since those are common margins used in projects. If you find the guides unpleasant and distracting, I still suggest enabling them while working on this project at least to help keep the wrapping correct.

michaelhixson · 2020-01-22T19:05:17Z

src/main/java/tfb/status/handler/ShareResultsPageHandler.java

+  }
+
+  @Override
+  public void handleRequest(HttpServerExchange exchange) throws Exception {


Please narrow the exception type of this method if possible. Can it be declared to only throw IOException?

michaelhixson · 2020-01-22T19:10:22Z

src/main/java/tfb/status/service/ShareResultsUploader.java

+      Path permanentFile = fileStore.shareDirectory().resolve(zipFileName);
+      MoreFiles.createParentDirectories(permanentFile);
+
+      try (FileOutputStream fos = new FileOutputStream(permanentFile.toFile());


Avoid calling Path.toFile(), which uses java.io.File. I suggest avoiding the ZipOutputStream class completely and use the java.nio.file APIs instead.

try (FileSystem zipFs = Files.newFileSystem(permanentFile)) { Path entry = zipFs.getPath(fileName); try (InputStream in = Files.newInputStream(tempFile); OutputStream out = Files.newOutputStream(entry, CREATE_NEW)) { in.transferTo(out); } }

Here's a slight correction to that example code for creating the zip file.

try (FileSystem zipFs = FileSystems.newFileSystem( permanentFile, Map.of("create", "true")) { ... }

jdk.zipfs module documentation

michaelhixson · 2020-01-22T19:14:29Z

src/main/java/tfb/status/service/ShareResultsUploader.java

+    long shareDirectorySize = FileStore.directorySizeBytes(
+        fileStore.shareDirectory().toFile());
+    if (shareDirectorySize >= fileStoreConfig.maxShareDirectorySizeBytes) {
+      throw new ShareResultsUploadException("Share uploads has reached max capacity.");


Have you thought about what we (the maintainers of tfb-status) should do in this case?

We may want to audit the share directory and ensure it didn't get to capacity due to someone trying to break or exploit this feature. Depending, we can delete data from such a user, or expand the capacity if we feel it's being used to good end. If the oldest files are quite old, we can go the route of backing up then deleting old files from the share directory. Does that make sense? Also, should this send some sort of alert email if the capacity is hit?

An alert email would be nice. Could you add that functionality along with a test? We'd need spam prevention measures, perhaps something like the one-email-per-day check implemented by RunCompleteMailer.

I suppose a manual solution is best with respect to not losing data. In practice, I'm not sure how I'd tell whether the uploaded results were legitimate or not. I might address the problem by simply deleting the share directory.

Please make the email message include at least one suggestion for what we should do to fix the problem.

I will add the email alert and related testing.

We could look at how closely together in time the results were uploaded, and if all of the results uploaded are identical. However these would be obvious signs that a user could get around. Simply deleting the share directory would fine in either case.

- Make the default urls production values rather than local - Better UrlsConfig variables documentation - Better ShareResultsHandler documentation - Fix incorrect required=true in deserialization annotations

…guide

and also use different paths

- separate handler for form submission type uploads of files - don't show exception messages to the user - share form is submitted by ajax

…tems

michaelhixson · 2020-01-29T22:29:32Z

src/main/java/tfb/status/view/Results.java

@@ -117,6 +117,13 @@
   */
  public final @Nullable GitInfo git;

+  /**
+   * The test metadata, generated during this run (same as test_metadata.json).
+   * Test metadata was not always included in the results, so this will be


This currently has the wrong name in TFB and you're planning to rename it, right? So we don't have an exact date for when it was added yet?

This PR fixed the naming, it was merged Jan 24. TechEmpower/FrameworkBenchmarks#5424

In that case, can you add a message like this to the documentation for this field?

michaelhixson · 2020-01-29T22:37:28Z

src/main/java/tfb/status/view/ShareResultsPageView.java

+   * "checked". This allows mustache template to simply render the radio
+   * button as checked without having to do any if checks.
+   */
+  public final ImmutableMap<String, String> shareTypeValues;


Is this used? I'm not seeing it.

michaelhixson · 2020-01-29T22:47:21Z

src/test/java/tfb/status/handler/ShareResultsViewHandlerTest.java

+
+    Results results = resultsTester.newResults();
+
+    Path jsonFile = fileSystem.getPath("results_to_upload.json");


I have an eventual goal to allow all the tests to run concurrently. I think this code here may be modeled after an existing test that did something similar using the same filename, but as a result, the two tests would conflict with each other if ran concurrently, wouldn't they? Could you prepend the test class name and/or test method name to this filename, or figure out some other scheme that's likely to keep this non-conflicting with other tests?

I made them use random UUIDs, deos that work? If that's messy or not right I can update to use class name + test method name.

That's fine.

michaelhixson · 2020-01-29T23:14:42Z

The PR is looking good now aside from the feature request for a new email and the other couple of minor comments I just left. There are some minor style inconsistencies related to existing code, but it's easier for me to edit those things later myself.

…etween tests, and fix a line that was over 80 column width

… tries to share

smathews-techempower · 2020-01-30T22:29:49Z

src/test/java/tfb/status/service/ShareResultsMailerTest.java

+
+    messages = getShareDirectoryFullEmails(mailServer);
+
+    assertEquals(initialMessagesSize + 1, messages.size());


This works now, but may cause an issue if you make the tests run in parallel. One of the other tests tries to share a results file after filling the share directory to capacity, which also sends this email. I'm curious what the right solution would be. One idea is that since the email contains the capacity and size in the body, I could add code to find a message with the specific numbers passed to onShareDirectoryFull above.

Let's not solve this now, but please annotate the test so that it can't run in parallel and add a TODO near the part of the test that's unfriendly to parallelism.

… a TODO

smathews-techempower added 6 commits January 2, 2020 18:33

Fix errors from merge conflicts, add comments

96d5204

Add comments and use the fixed testMetadata key from results files

4a3f6e8

Add missing class-level javadoc

e16c3ae

smathews-techempower requested a review from michaelhixson January 22, 2020 00:08

michaelhixson suggested changes Jan 22, 2020

View reviewed changes

smathews-techempower added 11 commits January 22, 2020 11:31

Use lowerCamelCase for consistency with the other settings

9f24d97

- Revert the local port to 80 rather than 8080

f212d02

- Make the default urls production values rather than local - Better UrlsConfig variables documentation - Better ShareResultsHandler documentation - Fix incorrect required=true in deserialization annotations

Imports: use the ordering and spacing rules from Google's Java style …

f97fadc

…guide

Add final to classes where possible

4ce7191

Split the ShareResultsHandler into 2 separate handlers for GET/POST,

5f15213

and also use different paths

Use Input/Output Streams instead of Readable/Writable Channels

9c94bea

Improve paste bin results sharing:

e283f11

- separate handler for form submission type uploads of files - don't show exception messages to the user - share form is submitted by ajax

Create dedicated FileUtils, avoid using java.io.File, and use FileSys…

50a98e2

…tems

Fix line break in comment at incorrect place

7e7ed26

Remove config.yml that was accidentally committed

1437142

Add tests for new share results features

9e6408f

michaelhixson reviewed Jan 29, 2020

View reviewed changes

Make new share results test classes final

aeb48e9

michaelhixson reviewed Jan 29, 2020

View reviewed changes

Make new classes final, remove unused class

e6d1053

michaelhixson reviewed Jan 29, 2020

View reviewed changes

smathews-techempower added 3 commits January 29, 2020 15:17

Clarify when testMetadata was added to results json

e4d1c13

Use random uuids for json files during testing to prevent conflicts b…

def434e

…etween tests, and fix a line that was over 80 column width

Send an email once a day when the share directory is full and someone…

4e3dbfe

… tries to share

smathews-techempower commented Jan 30, 2020

View reviewed changes

Ensure ShareResultsMailerTest the test is not run in parallel and add…

e9e22ad

… a TODO

michaelhixson merged commit 29d22a2 into TechEmpower:master Jan 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add public share results feature #20

Add public share results feature #20

smathews-techempower commented Jan 22, 2020 •

edited

Loading

michaelhixson left a comment

michaelhixson Jan 22, 2020

michaelhixson Jan 22, 2020

smathews-techempower Jan 22, 2020

michaelhixson Jan 22, 2020

michaelhixson Jan 22, 2020

michaelhixson Jan 22, 2020

michaelhixson Jan 22, 2020

michaelhixson Jan 22, 2020

michaelhixson Jan 22, 2020

michaelhixson Jan 23, 2020

michaelhixson Jan 22, 2020

smathews-techempower Jan 29, 2020

michaelhixson Jan 29, 2020

smathews-techempower Jan 30, 2020

michaelhixson Jan 29, 2020

smathews-techempower Jan 29, 2020

michaelhixson Jan 29, 2020

michaelhixson Jan 29, 2020

michaelhixson Jan 29, 2020

smathews-techempower Jan 30, 2020

michaelhixson Jan 30, 2020

michaelhixson commented Jan 29, 2020

smathews-techempower Jan 30, 2020

michaelhixson Jan 30, 2020


		Results results = resultsTester.newResults();

		Path jsonFile = fileSystem.getPath("results_to_upload.json");


		messages = getShareDirectoryFullEmails(mailServer);

		assertEquals(initialMessagesSize + 1, messages.size());

Add public share results feature #20

Add public share results feature #20

Conversation

smathews-techempower commented Jan 22, 2020 • edited Loading

michaelhixson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelhixson commented Jan 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smathews-techempower commented Jan 22, 2020 •

edited

Loading