feat(rest): provide more options to customize api explorer and openapi spec #1637

raymondfeng · 2018-08-23T01:29:12Z

The generated openapi spec has a dummy server url at the moment: servers: [url: '/']. The PR adds more options to rest server config.

See https://github.com/strongloop/loopback-next/blob/c78d636b9cccf66bc843ec49b203b0a6c319f78e/packages/rest/README.md#configuration

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

bajtos · 2018-08-23T13:31:27Z

What's wrong with using a path only in the server url (servers: [url: '/'])? This is not a dummy value but an intentional design that prevents all kinds of issues in deployments where the application does not know its public hostname and port.

Please note that OpenAPI spec allows relative URLs like the one we are using. See https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.1.md#serverObject

url - string - REQUIRED
A URL to the target host. This URL supports Server Variables and MAY be relative, to indicate that the host location is relative to the location where the OpenAPI document is being served. Variable substitutions will be made when a variable is named in {brackets}.

I am personally against landing this change and prefer to look into ways how to fix OpenAPI consumers to correctly support relative URLs.

raymondfeng · 2018-08-23T15:34:13Z

@bajtos Good point. Let's evaluate a bit more.

virkt25 · 2018-08-24T02:46:14Z

From what I remember the OASGraph team said this was not a critical fix unlike the responses field and required: true for path parameters.

bajtos · 2018-08-24T07:45:40Z

Can we perhaps make the server URL configurable?

By default, use a relative URL as we do now (/)
Add an option to use the URL inferred from the app config, the request properties and X-Forwarded-* headers.
Add an option (perhaps the same one as above?) allowing the user to provide their own protocol+hostname+port.

Thoughts?

raymondfeng · 2018-08-24T14:40:46Z

By default, use a relative URL as we do now (/)

+1.

Add an option to use the URL inferred from the app config, the request properties and X-Forwarded-* headers.

+1.

Add an option (perhaps the same one as above?) allowing the user to provide their own protocol+hostname+port.

server.api() allows the customization of the initial openapi spec, which can include servers.

raymondfeng · 2018-08-24T16:41:52Z

@bajtos @virkt25 I have introduced more options for the REST server. PTAL.

raymondfeng · 2018-08-28T15:30:01Z

@bajtos PTAL

virkt25 · 2018-08-29T18:08:59Z

packages/rest/README.md

+      setServersFromRequest: false,
+      // Template the OpenAPI spec, which will be filled with `paths` from
+      // controllers.
+      openApiSpec: {


I think the base template for OpenAPI spec should not be set in the apiExplorer object. It should be it's own top level property under rest.

Good point. Fixed.

virkt25 · 2018-08-29T18:31:16Z

packages/rest/README.md

+| ----------- | ------------------ | --------------------------------------------------------------------------------------------------------- |
+| port        | number             | Specify the port on which the RestServer will listen for traffic.                                         |
+| sequence    | SequenceHandler    | Use a custom SequenceHandler to change the behavior of the RestServer for the request-response lifecycle. |
+| apiExplorer | ApiExplorerOptions | Custom how API explorer and OpenAPI spec is served                                                        |


Please document openApiSpec property in this table. :)

virkt25 · 2018-08-29T18:54:30Z

packages/rest/README.md

+      // default to https://loopback.io/api-explorer
+      explorerUrl: 'http://petstore.swagger.io',
+      // Set `servers` based on HTTP request headers, default to `false`
+      setServersFromRequest: false,


I don't think this is a apiExplorer related property either but now sure what a better place for this might be.

How about moving this property from rest.apiExplorer.setServersFromRequest to something like rest.updateOpenApiServersFromRequest?

Uh, rest.updateOpenApiServersFromRequest is awfully long. How about rest.setServerUrlsFromRequest?

Or how about just rest.serverUrlsFromRequest?

bajtos

The changes look much better now, let's discuss the details bit more.

bajtos · 2018-08-30T11:40:27Z

packages/rest/README.md

+      // default to https://loopback.io/api-explorer
+      explorerUrl: 'http://petstore.swagger.io',
+      // Set `servers` based on HTTP request headers, default to `false`
+      setServersFromRequest: false,


How about moving this property from rest.apiExplorer.setServersFromRequest to something like rest.updateOpenApiServersFromRequest?

bajtos · 2018-08-30T11:45:32Z

packages/rest/README.md

+
+    // Template of the OpenAPI spec, which will be filled with `paths` from
+    // controllers.
+    openApiSpec: {


I believe we already have an API for setting the initial OpenAPI spec values, see http://apidocs.loopback.io/@loopback%2fdocs/rest.html#RestApplication.prototype.api

Can we keep using app.api() instead of introducing this new option? If not, then what is your reasoning? What benefits do you see in this declarative approach?

The current downside of app.api() is that the user has to provide spec.openapi and spec.paths properties in addition to info.

app.api({ openapi: '3.0.1', info: { /*...*/ }, servers: [/*...*/], paths: {}, });

On the brighter side, app.api() allows TypeScript to verify the correctness of the passed values. I am not sure if the type checks work for your declarative approach, because the type used for options allow arbitrary (additional) properties.

We can relax the constraints on the argument of app.api() to allow any subset of OpenAPISpec and fill in openapi version and an empty paths object ourselves.

See also #1637 (comment)

Doesn't app.api() take in an entire spec vs. the new openApiSpec property which is intended to accept a base template for the spec rather than the complete spec itself (which would come from app.api)? I think the name openApiSpec doesn't indicate clearly that it's intended to be just a template.

I believe our current code merges the following sources:

the spec provided by app.api() (if any)

specs provided at controller level via @api decorator,

specs built from controller-method-level decorators like @get and @param

specs provided via app.route(new Route(verb, path, spec, ...))

I don't mind providing a declarative way for providing a base template for the OpenAPI spec, perhaps in addition to app.api() API. However, I think we need to move that declarative config to a different place than application/server config, see #1637 (comment).

bajtos · 2018-08-30T11:46:21Z

packages/rest/README.md

+    apiExplorer: {
+      // URL for the hosted API Explorer UI
+      // default to https://loopback.io/api-explorer
+      explorerUrl: 'http://petstore.swagger.io',


Can we use a shorter name url? The fact that this setting is controlling API Explorer is already encoded in the parent property name apiExplorer.

bajtos · 2018-08-30T11:47:03Z

packages/rest/README.md

+| ----------- | ------------------ | --------------------------------------------------------------------------------------------------------- |
+| port        | number             | Specify the port on which the RestServer will listen for traffic.                                         |
+| sequence    | SequenceHandler    | Use a custom SequenceHandler to change the behavior of the RestServer for the request-response lifecycle. |
+| openApiSpec | OpenApiSpec        | Base template of the OpenAPI spec                                                                         |


This is not entirely correct. OpenApiSpec type requires openapi version and paths object to be present.

bajtos · 2018-08-30T11:48:54Z

packages/rest/src/rest.server.ts

   */
  public requestHandler: HttpRequestListener;

+  protected _config: RestServerConfig;


IMO, this property should not be modified after the constructor has finished, let's mark it as readonly please.

Is there any particular reason for keeping the config protected? I mean can we make it public?

bajtos · 2018-08-30T11:52:42Z

packages/rest/src/rest.server.ts

-    if (options.format === 'json') {
+    if (
+      this._config.apiExplorer &&
+      this._config.apiExplorer.setServersFromRequest


To avoid repetitive check for this._config.apiExplorer, I am proposing to always initialize this property in the constructor, so that it's either an empty object or the user-provider object. Then we can simplify this part as follows:

if (this._config.apiExplorer.setServersFromRequest)

See also https://loopback.io/doc/en/contrib/style-guide.html#indentation-of-multi-line-expressions-in-if

The style/formatting is enforced by prettier.

bajtos · 2018-08-30T11:53:18Z

packages/rest/src/rest.server.ts

-      request,
-      options,
-    )}/openapi.json`;
+      (this._config.apiExplorer && this._config.apiExplorer.explorerUrl) ||


To avoid repetitive check for this._config.apiExplorer, I am proposing to always initialize this property in the constructor, so that it's either an empty object or the user-provider object. Then we can simplify this part as follows:

const baseUrl = this._config.apiExplorer.explorerUrl || 'https://loopback.io/api-explorer';

We can even set explorerUrl to the default value in the constructor, that would allow us to simplify this code even further.

bajtos · 2018-08-30T11:55:10Z

packages/rest/test/integration/rest.server.integration.ts

+      rest: {
+        port: 0,
+      },
+    });


This looks like a change of formatting only, can we revert it please? Unless there is a reason for this change, in which case I'd like to hear it :)

I use prettier to format the code

I believe prettier allow multiple formatting styles these days, depending on how you write the code. I.e. it preserves one-liner {rest: {port: 0}} if you started with it, or keeps multi-line version if that's what the developer wrote.

bajtos · 2018-08-30T11:56:21Z

packages/rest/test/integration/rest.server.integration.ts

+      },
+    });
+    expect(response.get('Access-Control-Allow-Origin')).to.equal('*');
+    expect(response.get('Access-Control-Allow-Credentials')).to.equal('true');


IIUC, this test is verifying how rest.openApiSpec config is applied. There is no need to check CORS headers in such case, they have been already verified by previous tests.

Please remove these two assertions.

bajtos · 2018-08-30T11:56:35Z

packages/rest/test/integration/rest.server.integration.ts

+    });
+    expect(response.body.servers[0].url).to.match(/http:\/\/127.0.0.1\:\d+/);
+    expect(response.get('Access-Control-Allow-Origin')).to.equal('*');
+    expect(response.get('Access-Control-Allow-Credentials')).to.equal('true');


Ditto.

IIUC, this test is verifying how rest.openApiSpec config is applied. There is no need to check CORS headers in such case, they have been already verified by previous tests.

Please remove these two assertions.

bajtos · 2018-08-30T12:05:01Z

On the second thought, my main objection against providing OpenAPI spec in RestServerConfig (and ApplicationConfig) (see #1637 (comment)) is that the configuration of application behavior/operations should not be something that server/application users are allowed to change!

This something we have already discussed in the past, see #742.

Right now, Application constructor accepts a config/options object that mixes two very different concerns:

The functionality/features the application should implement - what controllers it contains, what repositories are registered, etc. ("the assembly instructions")

The configuration - what port to listen on, what database servers to connect to, etc.

IMO, the consumers of an application must not change its functionality and features (with the exception of enabling/disabling explicit feature flags) and therefore the config/options argument should contain only the configuration (port numbers, connection strings, feature flags).

The ApplicationConfig class should contain only things like HTTP port and database connection strings, there should be no entries related to what component, controller, repositories (etc.) the application is using. User application classes should call this.controller(), this.repository() (etc.) to register their artifacts, and eventually use the new bootstrapper module.

In this light, if we want to make OpenAPI servers field configurable via app/server options, then only the array of ServerObjects should be accepted. For example as rest.servers (instead of rest.openApi.servers.

raymondfeng · 2018-08-30T17:13:10Z

@bajtos I made some changes based on your feedback. The config still allows rest.openApiSpec.template but I constrain it from taking paths and components so that it becomes a pure configuration object. PTAL.

bajtos

Thank you for addressing my comments, the proposal looks so much better now!

I am not convinced that the info object is something that application consumers should be allowed to change. For example, isn't info.version always controlled by the application?

My proposal: allow users to configure server arrays only. (Remove rest.openApiSpec.template, add rest.openApiSpec.servers).

Thoughts?

bajtos · 2018-08-31T07:36:56Z

packages/rest/test/integration/rest.server.integration.ts

+      rest: {
+        port: 0,
+      },
+    });


I believe prettier allow multiple formatting styles these days, depending on how you write the code. I.e. it preserves one-liner {rest: {port: 0}} if you started with it, or keeps multi-line version if that's what the developer wrote.

bajtos · 2018-08-31T07:39:27Z

packages/rest/src/rest.server.ts

-      options,
-    )}/openapi.json`;
+      (this.config.apiExplorer && this.config.apiExplorer.url) ||
+      'https://loopback.io/api-explorer';


Can you set the default value for apiExplorer.url inside RestServer constructor please? Not only it will simplify the code here, but it will make it easier for 3rd parties to introspect the API Explorer configuration and obtain the actual URL used even when no custom value was configured by the user.

bajtos · 2018-08-31T07:57:11Z

packages/rest/README.md

  rest: {
    port: 3001,
+
+    // Template of the OpenAPI spec, which will be filled with `paths` from


I am proposing to move this new documentation to https://github.com/strongloop/loopback-next/blob/master/docs/site/Server.md and modify this README with a link pointing to that doc page.

For example, configuration of HTTPS is already described in Server.md only, see here.

Let's consolidate the README.md with Server.md in a separate PR.

the comments have been addressed

raymondfeng · 2018-08-31T17:24:20Z

@bajtos PTAL

bajtos · 2018-09-03T12:49:10Z

packages/rest/src/rest.server.ts

+   * security imposed by browsers as the spec is exposed over `http` by default.
+   * https://github.com/strongloop/loopback-next/issues/1603
+   */
+  urlForHttp?: string;


I would expect LoopBack to be smart enough to figure out that urlForHttp is the same as url, but using http:// instead of https://.

What are the use cases you have in mind where urlForHttp would significantly differ from url?

urlForHttp is optional and we can infer it from url in most cases. I added it there since we logically need two values and have to store both of them anyway.

How about httpUrl?

bajtos · 2018-09-03T12:53:07Z

packages/rest/README.md

    port: 3001,
+
+    // Optional configuration for how to serve OpenAPI spec
+    openApiSpec: {


IMO, this configuration example has too many comments to make it practical. That's why I proposed to move this content to https://github.com/strongloop/loopback-next/blob/master/docs/site/Server.md, where you can split the code into multiple sections/paragraph and use regular text instead of code comments.

I am fine with your proposal to leave consolidation of README out of scope of this pull request.

What I would like to ask you is to move this new README content to Server.md right now, so that we don't introduce more work for the future consolidation effort; plus reformat the documentation of config options into a form that's easier to read. It would be great to give a wider context to the readers while you are at this, e.g. why and when to change openApiSpec configuration, etc.

I'll clean up the docs.

b-admike

I have one comment, otherwise LGTM 👍

b-admike · 2018-09-04T15:53:47Z

packages/rest/test/integration/rest.server.integration.ts

+  });
+
+  it('exposes "GET /swagger-ui" endpoint with apiExplorer.urlForHttp', async () => {
+    const server = await givenAServer({


Sorry maybe I'm missing something, but where are the assertions for this test case?

See below:

const expectedUrl = new RegExp( [ 'http://petstore.swagger.io', '\\?url=http://\\d+.\\d+.\\d+.\\d+:\\d+/openapi.json', ].join(''), ); expect(response.get('Location')).match(expectedUrl);

The url is now 'http://petstore.swagger.io

raymondfeng · 2018-09-04T19:54:53Z

@bajtos Moved REST server configuration to docs/site/Server.md. PTAL.

virkt25 · 2018-09-05T14:12:46Z

packages/rest/src/rest.server.ts

      // spec to be converted into an XML response.
-      const settings = OPENAPI_SPEC_MAPPING[request.url];
-      return this._serveOpenApiSpec(request, response, settings);
+      const form = mapping[request.url];


Why is this called form? Seems unintuitive.

What's a better name for version + format?

virkt25 · 2018-09-05T14:14:12Z

packages/rest/src/rest.server.ts

+    config.apiExplorer.url =
+      config.apiExplorer.url || 'https://loopback.io/api-explorer';
+
+    config.apiExplorer.urlForHttp =


I think httpUrl might be a better name.

virkt25 · 2018-09-05T14:15:07Z

packages/rest/src/rest.server.ts

+   * security imposed by browsers as the spec is exposed over `http` by default.
+   * https://github.com/strongloop/loopback-next/issues/1603
+   */
+  urlForHttp?: string;


How about httpUrl?

virkt25

Looks great!

virkt25 · 2018-09-06T02:37:11Z

packages/rest/src/rest.server.ts

      // spec to be converted into an XML response.
-      const settings = OPENAPI_SPEC_MAPPING[request.url];
-      return this._serveOpenApiSpec(request, response, settings);
+      const specForm = mapping[request.url];


Great name!

bajtos

The documentation looks so much better now 👍

Let's clean up the test code a bit, see my comments below.

Additionally, please a test to verify how we treat IPv6 addresses, see #1623

bajtos · 2018-09-07T11:38:32Z

packages/rest/test/integration/rest.server.integration.ts

+        version: '1.0.0',
+      },
+      servers: [{url: 'http://127.0.0.1:8080'}],
+      paths: {


Please simplify this assertion to focus only on the part that's important (i.e. servers property) and leave out the rest of the API spec that's only adding noise to error messages and test source code.

expect(response.body.servers).to.deepEqual([ {url: 'http://127.0.0.1:8080'}, ]);

bajtos · 2018-09-07T11:40:51Z

packages/rest/test/integration/rest.server.integration.ts

+      },
+    });
+    const greetSpec = {
+      responses: {


~~This OpenAPI snippet is repeated in many tests. Please extract it into a helper function, e.g a function that adds a dummy route.~~~

Do we really need a route to verify how top-level spec properties are handled? Isn't in enough to render the spec for an empty app? Such app should still produce servers property of the spec. See the test case "exposes endpoints with openApiSpec.endpointMapping" for an example.

Leaving out the route we don't need in the test will make the test code much simpler and easier to understand. Please remove it.

bajtos · 2018-09-07T11:42:10Z

packages/rest/test/integration/rest.server.integration.ts

+        },
+      },
+    };
+    server.route(new Route('get', '/greet', greetSpec, function greet() {}));


I believe this route is not needed by this test, could you please remove?

bajtos · 2018-09-07T11:42:37Z

packages/rest/test/integration/rest.server.integration.ts

+        },
+      },
+    };
+    server.route(new Route('get', '/greet', greetSpec, function greet() {}));


I believe this route is not needed to verify that "setServersFromRequest" is handled correctly, could you please remove it?

raymondfeng · 2018-09-07T16:43:28Z

@bajtos Your comments are addressed:

Clean up tests
Add a test for ipv6 url construction
Cherry-pick the code from [WIP][RFC] Add local swagger ui for LoopBack 4 API explorer #1664 to mount endpoints for OpenAPI specs

PTAL.

hacksparrow · 2018-09-10T08:48:04Z

packages/rest/src/rest.server.ts

+    config.apiExplorer.url =
+      config.apiExplorer.url || 'https://loopback.io/api-explorer';
+
+    config.apiExplorer.httpUrl =


Can we come up with a more intuitive interface/names for apiExplorer.url and apiExplorer.httpUrl?

httpUrl makes me wonder if there would be httpsUrl as well.

I know it is not trivial and might get overly complicated for a simple thing, but I'd like the ability to set a HTTP url and HTTPS url, and mark one as the default.

On second thoughts, I am fine with this. HTTPS is starting to become the expected default in browsers, let HTTPS be the default apiExplorer.url.

bajtos

LGTM

raymondfeng requested a review from bajtos as a code owner August 23, 2018 01:29

virkt25 approved these changes Aug 23, 2018

View reviewed changes

b-admike approved these changes Aug 23, 2018

View reviewed changes

raymondfeng force-pushed the fix-openapi-server-url branch from 9dcd3a6 to c78d636 Compare August 24, 2018 16:39

raymondfeng changed the title ~~fix(rest): set default server url for openapi specs~~ feat(rest): provide more options to customize api explorer and openapi spec Aug 24, 2018

raymondfeng force-pushed the fix-openapi-server-url branch 3 times, most recently from 01258fb to 944b06e Compare August 27, 2018 16:28

virkt25 reviewed Aug 29, 2018

View reviewed changes

raymondfeng force-pushed the fix-openapi-server-url branch from 944b06e to 8f3c135 Compare August 29, 2018 18:27

virkt25 reviewed Aug 29, 2018

View reviewed changes

raymondfeng force-pushed the fix-openapi-server-url branch from 8f3c135 to 41b15e6 Compare August 29, 2018 19:18

bajtos previously requested changes Aug 30, 2018

View reviewed changes

raymondfeng force-pushed the fix-openapi-server-url branch 2 times, most recently from d203959 to bcd609a Compare August 30, 2018 17:11

bajtos reviewed Aug 31, 2018

View reviewed changes

raymondfeng force-pushed the fix-openapi-server-url branch from bcd609a to 41b4bfd Compare August 31, 2018 17:22

raymondfeng force-pushed the fix-openapi-server-url branch from 41b4bfd to 6a78b9f Compare August 31, 2018 21:46

raymondfeng force-pushed the fix-openapi-server-url branch 2 times, most recently from 7fe7531 to d58c107 Compare September 1, 2018 02:34

bajtos reviewed Sep 3, 2018

View reviewed changes

b-admike reviewed Sep 4, 2018

View reviewed changes

raymondfeng force-pushed the fix-openapi-server-url branch 2 times, most recently from 828295d to 154f434 Compare September 4, 2018 19:53

virkt25 reviewed Sep 5, 2018

View reviewed changes

raymondfeng force-pushed the fix-openapi-server-url branch from 154f434 to d2572f3 Compare September 5, 2018 21:47

virkt25 approved these changes Sep 6, 2018

View reviewed changes

dhmlau mentioned this pull request Sep 6, 2018

Swagger-ui redirect fails when on IPv6 locally #1623

Closed

1 task

bajtos suggested changes Sep 7, 2018

View reviewed changes

bajtos mentioned this pull request Sep 7, 2018

[WIP][RFC] Add local swagger ui for LoopBack 4 API explorer #1664

Closed

7 tasks

raymondfeng force-pushed the fix-openapi-server-url branch from d2572f3 to b56fb37 Compare September 7, 2018 15:58

feat(rest): make servers configurable for openapi specs

eca79a4

raymondfeng force-pushed the fix-openapi-server-url branch from b56fb37 to eca79a4 Compare September 7, 2018 16:36

dhmlau assigned raymondfeng Sep 8, 2018

hacksparrow reviewed Sep 10, 2018

View reviewed changes

bajtos approved these changes Sep 10, 2018

View reviewed changes

raymondfeng merged commit 99b80a9 into master Sep 10, 2018

bajtos mentioned this pull request Sep 25, 2018

feat: add a default home page loopbackio/loopback4-example-shopping#16

Merged

bajtos deleted the fix-openapi-server-url branch October 4, 2018 06:48

bajtos mentioned this pull request Jan 31, 2019

[Spike] Deploy a basic LB4 application to IBM Cloud Kubernetes #1606

Closed

3 tasks

feat(rest): provide more options to customize api explorer and openapi spec #1637

feat(rest): provide more options to customize api explorer and openapi spec #1637

Uh oh!

Conversation

raymondfeng commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

bajtos commented Aug 23, 2018

Uh oh!

raymondfeng commented Aug 23, 2018

Uh oh!

virkt25 commented Aug 24, 2018

Uh oh!

bajtos commented Aug 24, 2018

Uh oh!

raymondfeng commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raymondfeng commented Aug 24, 2018

Uh oh!

raymondfeng commented Aug 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

virkt25 Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Aug 30, 2018

Uh oh!

raymondfeng commented Aug 23, 2018 •

edited

Loading

raymondfeng commented Aug 24, 2018 •

edited

Loading

virkt25 Aug 30, 2018 •

edited

Loading