Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Public API: Create new project (fixes #1095) #1106

Merged

Conversation

andrewn
Copy link
Member

@andrewn andrewn commented Jun 17, 2019

This implements the following new endpoints that match the Public API spec (#1076). I've modified npm run fetch-examples-ml5 so that it uses the new endpoints. It expects a user to exists and a Public Access Token to be provided to the script.

  • GET /api/:user/sketches to list all sketches
  • DELETE /api/:user/sketches/:id to delete a sketch
  • POST /api/:user/sketches to create a new sketch
  • Tests for the mapping of the API files data structure to the mongo models
  • Descriptive error messages returned by the API
  • Tests for the three controller functions

I need to add more tests around the controllers, but I wanted to share now to get feedback.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • is from a uniquely-named feature branch and has been rebased on top of the latest master. (If I was asked to make more changes, I have made sure to rebase onto master then too)
  • is descriptively named and links to an issue number, i.e. Fixes #123

The endpoints don't exist yet, but this is a good way to see how
the implementation of the data structures differ.
This transforms the nested tree of file data into a mongoose
Project model
@andrewn andrewn added this to the API and Collection Features milestone Jun 17, 2019
@andrewn andrewn requested a review from catarak June 17, 2019 12:38
server/server.js Outdated
@@ -95,6 +96,7 @@ app.use(session({

app.use(passport.initialize());
app.use(passport.session());
app.use('/api', requestsOfTypeJSON(), api);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these public endpoints should be mounted at /api and all the rest that only the editor uses should be mounted at /editor?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to me that sounds logical. do you think we should add the API versioning as well? i was trying to figure out what best practices are for this, but it seems like the options all have their own drawbacks (here's a blog post that breaks this down).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added URL-based versioning in 57ced9c. Although it's not ideal, I think it's more obvious for casual users that may write scripts to import data into the Editor.

I'll change the namespaces of the private and public APIs in another PR.

@@ -30,3 +31,27 @@ export default function createProject(req, res) {
.then(populateUserData)
.catch(sendFailure);
}

// TODO: What happens if you don't supply any files?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think there are two options here: (1) create default files (the ones that are currently the default when you click sketch-> new) or (2) the project isn't valid. my instinct here is (1)...

Copy link
Member Author

@andrewn andrewn Jun 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done. See ee6891d. The default files are created unless a root-level .html file is created. Also, if sketch.js and style.css exist, then they are not overwritten by defaults.

@@ -0,0 +1,35 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for writing this class and and moving towards better error handling/capturing ✨

@andrewn
Copy link
Member Author

andrewn commented Jun 19, 2019

@catarak Just pushed some tests and a check for slug uniqueness for the user. If a slug is specified and it is not unique in the sketches that the user has created, then the ids of the project(s) with identical slugs are returned as an error.

Eventually, I think this should live as a pre-save hook on the Project mongoose model. But as we don't have a UI for editing slugs, then we'd need to discuss what that behaviour should be first.

- implements tests
- update apiKey tests to use new User mocks
`message` is used as a high-level description of the errors
`detail` is optional and has an plain language explanation of the
individual errors
`errors` is an array of each individual problem from `detail` in a
machine-readable format
@andrewn andrewn force-pushed the feature/public-api-create-project branch from ba4ead7 to 3041b35 Compare July 4, 2019 13:04
andrewn and others added 18 commits July 22, 2019 16:16
Fixes a bug where the slug was auto-generated using the sketch name,
even if a slug property had been provided.
- implements tests
- update apiKey tests to use new User mocks
`message` is used as a high-level description of the errors
`detail` is optional and has an plain language explanation of the
individual errors
`errors` is an array of each individual problem from `detail` in a
machine-readable format
…n/p5.js-web-editor into feature/public-api-create-project
@andrewn
Copy link
Member Author

andrewn commented Aug 3, 2019

I only can't reproduce the slug validation issue, which is weird.

which version of mongo are you using? i'm using 3.6.6, since that's what mlab uses.

@catarak I still can't reproduce this. I dropped the p5js-web-editor mongo database and switched to using 3.6:

$ ps aux | grep mongo
andrew           11711   0.3  0.1  5106368  13004   ??  S    Mon09am  12:23.74 /usr/local/opt/mongodb-community@3.6/bin/mongod --config /usr/local/etc/mongod.conf

I created a new user (andrewn), with a new personal access token and POSTed to create a new sketch:

* Preparing request to http://localhost:8000/api/v1/andrewn/sketches
* Using libcurl/7.54.0 LibreSSL/2.6.5 zlib/1.2.11 nghttp2/1.24.1
* Current time is 2019-08-03T13:29:15.247Z
* Disable timeout
* Enable automatic URL encoding
* Disable SSL validation
* Enable cookie sending with jar of 0 cookies
* Found bundle for host localhost: 0x7fb7e1f28530 [can pipeline]
* Re-using existing connection! (#4) with host localhost
* Connected to localhost (::1) port 8000 (#4)
* Server auth using Basic with user 'andrewn'

> POST /api/v1/andrewn/sketches HTTP/1.1
> Host: localhost:8000
> Authorization: Basic YW5kcmV3bjpORFZrWVRCbU0ySXhOV1ExWTJVM05tTmxaamhrT1RWbVkyTXhPVEV6WkRJNU1EZzNPRGhqT0E9PQ==
> User-Agent: insomnia/6.5.4
> Content-Type: application/json
> Accept: */*
> Content-Length: 44

| {
|   "name": "My cute sketch",
| 	"files": {}
| }

* upload completely sent off: 44 out of 44 bytes

< HTTP/1.1 201 Created
< X-Powered-By: Express
< Vary: Origin
< Access-Control-Allow-Credentials: true
< Content-Type: application/json; charset=utf-8
< Content-Length: 18
< ETag: W/"12-7Z0Dd/UqbdLyYIPp5BjeLo/dpLU"
< Date: Sat, 03 Aug 2019 13:29:15 GMT
< Connection: keep-alive


* Received 18 B chunk
* Connection #4 to host localhost left intact

mongoose is at v 4.13.18:

$ npm ls mongoose
p5.js-web-editor@0.0.1 /Users/andrew/Projects/oss/processing/p5.js-web-editor
└── mongoose@4.13.18

Perhaps my mongod.conf is different?

@catarak
Copy link
Member

catarak commented Aug 6, 2019

let me try testing this again to sanity check, it's super weird!

@catarak
Copy link
Member

catarak commented Aug 6, 2019

i'm using postman to test this, maybe that has to do with it?

@catarak
Copy link
Member

catarak commented Aug 13, 2019

In testing this again, I found a bug. I made a POST to /api/v1/garbage_username/sketches with the correct authorization header (my real username, cassie + the API key), and a sketch body ({ "name": "My cute sketch", "files": {} }) and it successfully created a sketch under my username.

…mespace

Previously, the project was always created under the authenticated user's
namespace, but this not obvious behaviour.
@andrewn
Copy link
Member Author

andrewn commented Aug 30, 2019

In testing this again, I found a bug. I made a POST to /api/v1/garbage_username/sketches with the correct authorization header (my real username, cassie + the API key), and a sketch body ({ "name": "My cute sketch", "files": {} }) and it successfully created a sketch under my username.

This is now fixed. An UNAUTHORIZED response code is returned with the username of the authenticated user, and the username specified in the URL.

@catarak
Copy link
Member

catarak commented Aug 30, 2019

sweet, thanks for fixing that! i don't think there's any other issues so i'll merge this in.

@catarak catarak merged commit d44a058 into processing:feature/public-api Aug 30, 2019
andrewn added a commit to andrewn/p5.js-web-editor that referenced this pull request Sep 20, 2019
* Converts import script to use public API endpoints

The endpoints don't exist yet, but this is a good way to see how
the implementation of the data structures differ.

* Exposes public API endpoint to fetch user's sketches

* Implements public API delete endpoint

* Adds helper to create custom ApplicationError classes

* Adds create project endpoint that understand API's data structure

This transforms the nested tree of file data into a mongoose
Project model

* Returns '201 Created' to match API spec

* Removes 'CustomError' variable assignment as it shows up in test output

* transformFiles will return file validation errors

* Tests API project controller

* Tests toModel()

* Creates default files if no root-level .html file is provided

* Do not auto-generate a slug if it is provided

Fixes a bug where the slug was auto-generated using the sketch name,
even if a slug property had been provided.

* Validates uniqueness of slugs for projects created by the public API

* Adds tests for slug uniqueness

* Configures node's Promise implementation for mongoose (fixes warnings)

* Moves createProject tests to match controller location

* Adds support for code to ApplicationErrors

* deleteProject controller tests

* getProjectsForUser controller tests

- implements tests
- update apiKey tests to use new User mocks

* Ensure error objects have consistent property names

`message` is used as a high-level description of the errors
`detail` is optional and has an plain language explanation of the
individual errors
`errors` is an array of each individual problem from `detail` in a
machine-readable format

* Assert environment variables are provided at script start

* Version public API

* Expect "files" property to always be provided

* Fixes linting error

* Converts import script to use public API endpoints

The endpoints don't exist yet, but this is a good way to see how
the implementation of the data structures differ.

* Exposes public API endpoint to fetch user's sketches

* Implements public API delete endpoint

* Adds helper to create custom ApplicationError classes

* Adds create project endpoint that understand API's data structure

This transforms the nested tree of file data into a mongoose
Project model

* Returns '201 Created' to match API spec

* Removes 'CustomError' variable assignment as it shows up in test output

* transformFiles will return file validation errors

* Tests API project controller

* Tests toModel()

* Creates default files if no root-level .html file is provided

* Do not auto-generate a slug if it is provided

Fixes a bug where the slug was auto-generated using the sketch name,
even if a slug property had been provided.

* Validates uniqueness of slugs for projects created by the public API

* Adds tests for slug uniqueness

* Configures node's Promise implementation for mongoose (fixes warnings)

* Moves createProject tests to match controller location

* deleteProject controller tests

* Adds support for code to ApplicationErrors

* getProjectsForUser controller tests

- implements tests
- update apiKey tests to use new User mocks

* Ensure error objects have consistent property names

`message` is used as a high-level description of the errors
`detail` is optional and has an plain language explanation of the
individual errors
`errors` is an array of each individual problem from `detail` in a
machine-readable format

* Assert environment variables are provided at script start

* Version public API

* Expect "files" property to always be provided

* Fixes linting error

* Checks that authenticated user has permission to create under this namespace

Previously, the project was always created under the authenticated user's
namespace, but this not obvious behaviour.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants