http-server create new github issues. #4167

DanVanAtta · 2018-10-09T01:01:00Z

Overview

add a client for github web API to create github issues
update http-server to use this new client to create github issues from
error reports.

Functional Changes

Updates are all in http-client + http-server, both of which are non-prod.
if http-server is update to have a github personal access token with repo permissions (currently it is an empty string), then when using the error-upload API it will create issues in: https://github.com/triplea-game/test/issues

Manual Testing Performed

more extensively tested with other changes still in-flight that wired this to the front-end. Using front-end I was able to submit new issues. PRs coming to enable that 😄

Additional Review Notes

http server is re-arranged quite a bit to make new endpoint controllers easier to drop in.
http cilent similarly re-arranged. Of note http clients are now obtained through '*ClientFactory.java' classes, beans are wired and injected via '*Configuration.java' classes. 'HttpClient.java' is an interesting new class as service requests are wired automatically to send requests through its we can do common error classification and throttling.
new http client integrations should just need a feign interface and a boilerplate '*ClientFactory.java' to create it

- add a client for github web API to create github issues - update http-server to use this new client to create github issues from error reports.

codecov-io · 2018-10-09T01:10:03Z

Codecov Report

Merging #4167 into master will decrease coverage by 0.03%.
The diff coverage is 20.86%.

@@             Coverage Diff              @@
##             master    #4167      +/-   ##
============================================
- Coverage     22.62%   22.59%   -0.04%     
+ Complexity     6115     6109       -6     
============================================
  Files           857      871      +14     
  Lines         71337    71413      +76     
  Branches      11397    11398       +1     
============================================
- Hits          16140    16135       -5     
- Misses        53132    53209      +77     
- Partials       2065     2069       +4

Impacted Files	Coverage Δ	Complexity Δ
...ttp/client/throttle/rate/RateLimitingThrottle.java	`84.61% <ø> (ø)`	`3 <0> (ø)`	⬇️
...client/error/report/create/ErrorReportDetails.java	`80% <ø> (ø)`	`5 <0> (?)`
...trategy/debug/error/reporting/UserErrorReport.java	`58.33% <ø> (ø)`	`2 <0> (ø)`	⬇️
...n/java/org/triplea/server/ServerConfiguration.java	`0% <0%> (ø)`	`0 <0> (?)`
...a/org/triplea/server/EnvironmentConfiguration.java	`0% <0%> (ø)`	`0 <0> (?)`
...http/spark/controller/ControllerConfiguration.java	`0% <0%> (ø)`	`0 <0> (?)`
...a/http/client/error/report/create/ErrorReport.java	`97.61% <0%> (ø)`	`13 <0> (?)`
...in/java/org/triplea/http/client/ServiceClient.java	`0% <0%> (ø)`	`0 <0> (?)`
.../java/org/triplea/http/client/ServiceResponse.java	`55.55% <0%> (ø)`	`4 <0> (?)`
...porting/error/upload/ErrorUploadConfiguration.java	`0% <0%> (ø)`	`0 <0> (?)`
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 701491c...1d63a9b. Read the comment docs.

RoiEXLab

I haven't quite wrapped my head around the overall structure of the server side of things, but mostly the code changes seem resonable.
I noticed the usage of Gson, not quite familiar with it, did you test what happens when loading invalid/outdated JSON into a class?

General Questions

Once this project is finished, will a new issue be created for every user that uses this function?
If so, what are the odds this feature will be misused to spam the issue section?

RoiEXLab · 2018-10-09T14:05:29Z

http-client/src/test/java/org/triplea/http/client/error/reporting/WireMockSystemTest.java.orig

+<<<<<<< HEAD:http-client/src/test/java/org/triplea/http/client/WireMockSystemTest.java
+=======
+import org.triplea.http.client.ServiceCallResult;
+>>>>>>> Group error reporting http client classes together in a package:http-client/src/test/java/org/triplea/http/client/error/reporting/WireMockSystemTest.java


Errm I'm pretty sure that's a bad merge conflict resolution

Been a while since I checked in an extra file.. 😞

http-client/src/test/java/org/triplea/http/client/error/reporting/WireMockSystemTest.java.orig

In this case when adding entire new folders I did not notice the '.orig' files that resulted from resolving conflicts. A better self-review IMO should have caught that if not better inspection of the add list.

And even better, I also found the steps on stack-overflow to turn off the creation of '.orig' files 😄

File removed: 7590176

RoiEXLab · 2018-10-09T14:07:25Z

http-server/src/main/java/org/triplea/server/http/spark/controller/SparkController.java

+/**
+ * Marker interface for any spark component that will register an endpoint and associated handler.
+ */
+public interface SparkController extends Runnable {


Isn't this interface really unecessary

269006c removes the unnecessary "aliasing" interface. I'm compromising on this style in another project that it got a bit too ingrained 😟

ssoloff · 2018-10-09T15:32:53Z

Once this project is finished, will a new issue be created for every user that uses this function?
If so, what are the odds this feature will be misused to spam the issue section?

That's a good point. Also, we need to consider that GitHub will throttle our access to their API, so the server needs to be able to handle storing and retrying issue creation at a later time (or, if the server needs to remain truly stateless, report failure to submit the bug report to the user and tell them to try again later).

RoiEXLab · 2018-10-09T16:39:24Z

@ssoloff With a proper API Token we have plenty of requests to issue, I believe there are like 5000 request per hour, so we're not going to exceed this limit that quickly

ssoloff · 2018-10-09T16:54:12Z

@RoiEXLab Right. I thought it was 1,000 per hour, but whatever--same order of magnitude. 😄

I was raising the rate limiting issue specifically in response to your concern of an attack on the server. We can exceed 5,000 requests pretty easily if someone writes a script to attack the HTTP endpoint directly. We can't really rely on HTTP authentication to stop bogus submissions, right? Otherwise the TripleA client wouldn't be able to submit the report (unless we do some kind of OAuth integration with a provider or providers most players have an account with).

Maybe the server itself needs to rate limit reports by source IP? "Sorry, you can only submit 5 bug reports per hour." Once again, though, that will make the server stateful.

…ilt-in instead.

DanVanAtta · 2018-10-10T05:31:09Z

I haven't quite wrapped my head around the overall structure of the server side of things,

If it helps, I can try to explain the structure a bit. The class packages give it away a bit:

Above the main class is in http.spark, just the spark server and only depends on the controllers. The controllers are in an agnostic package as well, they are meant to "know http" and nothing about the backend or any of our "business logic". In part because I truly do want to put a parallel drop-wizard so we can really see how the two compare, there is a split of what is http request handling to what is processing logic.

The processing logic top level is in org.triplea.server, there we have objection instantiation and parameter configuration. Then the package underneath that is the gory bits of the processing logic for an error error report upload. New features in theory would get a new package in org.triplea.server and a new controller.

DanVanAtta · 2018-10-10T05:34:23Z

I'll note maybe the biggest changes were in http-client, the new 'factory' class with client were a result of working in a second http client. The github issue client is now the 2nd one. Those updates are somewhat easy to understand by looking at the package structure:

DanVanAtta · 2018-10-10T05:52:24Z

I noticed the usage of Gson, not quite familiar with it, did you test what happens when loading invalid/outdated JSON into a class?

You'll get an exception:

   * @throws JsonParseException if json is not a valid representation for an object of type typeOfT
   * @throws JsonSyntaxException if json is not a valid representation for an object of type

I've mostly used Jackson for JSON parsing, gson is pretty similar AFAIK, mostly a difference of whether properties are inferred from the 'getter' methods or from private properties. The 'feign' encoding/decode had a gson decode chosen, so just sticking with that same general set of libraries.

I do not know if out of the box whether gson will ignore or explode when it sees unrecognized properties.

Once this project is finished, will a new issue be created for every user that uses this function?

Good question, been wondering this one: #3949. Any version that stores reports in DB and has some sort of acceptance system get pretty convoluted soon, is hard to enlist help, is very custom, and could easily see starvation in bug reports. Overall, creating the report directly seemed to be a convenient and appropriate choice.

If so, what are the odds this feature will be misused to spam the issue section?

In general, low, over time, it'll probably happen. There is some client side rate limiting put in place, the server side will have rate limiting as well, it's not in place yet though (https://github.com/triplea-game/triplea/projects/4#card-12668058)

I wouldn't mind out of the scope of these changes a brainstorming session of the abuse-prevention items that we should build in. Happily github has mass issue delete in case we get flooded, but unlikely given the rate limit I suspect will be something like 10 per hour.

DanVanAtta · 2018-10-10T05:55:13Z

Maybe the server itself needs to rate limit reports by source IP? "Sorry, you can only submit 5 bug reports per hour." Once again, though, that will make the server stateful.

@ssoloff basically exactly that 😄 The state would be kept presumably in a DB table.

DanVanAtta · 2018-10-11T15:22:27Z

@RoiEXLab the updates you requested in code have been completed & pushed. Please take a look.

@RoiEXLab @ssoloff throttling and further rate limiting/abuse prevention is out of scope for now; the http-server is not even yet live/deployed. The main focus of this PR is that we have a new http integration to create issues and the backend server is wired to send those requests. I have more updates waiting in the wings for after this is merged. The throttling piece is being tracked in the project, it is not part of this PR intentionally.

RoiEXLab

Just some minor issues.
After that I'd leave it to @ssoloff to see if he has something additional to say.

RoiEXLab · 2018-10-11T15:39:27Z

http-client/src/main/java/org/triplea/http/client/FeignFactory.java

+@Log
+final class FeignFactory {
+  private static GsonEncoder gsonEncoder = new GsonEncoder();
+  private static GsonDecoder gsonDecoder = new GsonDecoder();


I don't think those variables are never getting reassigned, so making them final would be good

c0ce634

While there I also weakened the variable types.

RoiEXLab · 2018-10-11T15:45:49Z

http-server/src/test/java/org/triplea/server/http/spark/SparkServerSystemTest.java

 import org.triplea.test.common.Integration;

 import spark.Spark;


 @Integration
+@ExtendWith(MockitoExtension.class)


Correct me if I'm wrong, but I'm pretty sure this isn't necessary, because no mockito annotations are being used.

d4ebcba

You're right, it's only needed when using the annotations. 👍

http-server create new github issues.

21388dd

- add a client for github web API to create github issues - update http-server to use this new client to create github issues from error reports.

DanVanAtta mentioned this pull request Oct 9, 2018

Combine http-data module into http-client #4157

Closed

RoiEXLab requested changes Oct 9, 2018

View reviewed changes

DanVanAtta added 2 commits October 9, 2018 17:30

Delete leaked merge conflict backup file.

7590176

Remove unnecessary "aliasing" interface "SparkController", use the bu…

269006c

…ilt-in instead.

DanVanAtta mentioned this pull request Oct 10, 2018

Decide how to handle Reported Bugs #3971

Closed

DanVanAtta added 2 commits October 9, 2018 21:57

Merge remote-tracking branch 'upstream/master' into client-backend

5c9b46f

Fix merge resolutions

5cb21a2

DanVanAtta assigned RoiEXLab Oct 11, 2018

RoiEXLab reviewed Oct 11, 2018

View reviewed changes

DanVanAtta requested a review from ssoloff October 12, 2018 04:10

DanVanAtta added 3 commits October 11, 2018 21:11

Mark final and weaken type

c0ce634

Remove unnecessary "ExtendWith" mockito annotation

d4ebcba

Fix unused imports

1d63a9b

ssoloff approved these changes Oct 13, 2018

View reviewed changes

ssoloff merged commit 16791d9 into triplea-game:master Oct 13, 2018

ssoloff unassigned RoiEXLab Oct 13, 2018

DanVanAtta deleted the client-backend branch October 15, 2018 05:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http-server create new github issues. #4167

http-server create new github issues. #4167

DanVanAtta commented Oct 9, 2018

codecov-io commented Oct 9, 2018 •

edited

Loading

RoiEXLab left a comment

RoiEXLab Oct 9, 2018

DanVanAtta Oct 10, 2018 •

edited

Loading

DanVanAtta Oct 10, 2018

RoiEXLab Oct 9, 2018

DanVanAtta Oct 10, 2018

ssoloff commented Oct 9, 2018

RoiEXLab commented Oct 9, 2018

ssoloff commented Oct 9, 2018

DanVanAtta commented Oct 10, 2018

DanVanAtta commented Oct 10, 2018

DanVanAtta commented Oct 10, 2018

DanVanAtta commented Oct 10, 2018

DanVanAtta commented Oct 11, 2018

RoiEXLab left a comment

RoiEXLab Oct 11, 2018

DanVanAtta Oct 12, 2018

RoiEXLab Oct 11, 2018

DanVanAtta Oct 12, 2018

http-server create new github issues. #4167

http-server create new github issues. #4167

Conversation

DanVanAtta commented Oct 9, 2018

Overview

Functional Changes

Manual Testing Performed

Additional Review Notes

codecov-io commented Oct 9, 2018 • edited Loading

Codecov Report

RoiEXLab left a comment

Choose a reason for hiding this comment

General Questions

Choose a reason for hiding this comment

DanVanAtta Oct 10, 2018 • edited Loading

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

ssoloff commented Oct 9, 2018

RoiEXLab commented Oct 9, 2018

ssoloff commented Oct 9, 2018

DanVanAtta commented Oct 10, 2018

DanVanAtta commented Oct 10, 2018

DanVanAtta commented Oct 10, 2018

DanVanAtta commented Oct 10, 2018

DanVanAtta commented Oct 11, 2018

RoiEXLab 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

codecov-io commented Oct 9, 2018 •

edited

Loading

DanVanAtta Oct 10, 2018 •

edited

Loading