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

generate methods options for .serve function #386

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pimvanderheijden
Copy link

I added the feature mentioned in issue #385 .

Am encountering two issues:

  • On master, 3 tests for findOneAndUpdate: true with 400 - mongo error are not passing.
  • With my changes, the count routes seem to be created on the app object but it's middleware functions aren't called with making the request. Need some help here...

@pimvanderheijden
Copy link
Author

I see now that the findOneAndUpdate: true with 400 - mongo error tests are passing in the Travis CI. So, they somehow only fail on my machine.

@Zertz
Copy link
Collaborator

Zertz commented Aug 15, 2018

Awesome! I'll take a look at the tests, the one's checking for errors depend on the specific MongoDB/Mongoose versions so as long as Travis passes, we're fine.

@pimvanderheijden
Copy link
Author

Shall I have another look at this?

@Zertz
Copy link
Collaborator

Zertz commented Apr 10, 2019

Since the package is so closely coupled to MongoDB, would it make sense to align the method names with Mongo's?

get -> find
getone -> findOne

And so on? I'm not sure myself!

I might also be tempted to shorten generateMethods to methods and eventually we can move to a structure where every method is opt-in. In the next major version we could make the option mandatory (more of a setting actually) so users have to intentionally enable endpoints and be in a situation where you forget to secure a particular method and end up with an invisible security issue.

Also, just a small thing, I'd rather we stick to prettier defaults so we can focus on code and not style 😸

let app = createFn()
let server

before(done => {
Copy link
Owner

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace done with (done)

Suggested change
before(done => {
before((done) => {

dismantle(app, server, done)
})

it('GET /Customer 200', done => {
Copy link
Owner

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace done with (done)

Suggested change
it('GET /Customer 200', done => {
it('GET /Customer 200', (done) => {

let server

before(done => {
setup(err => {
Copy link
Owner

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace err with (err)

Suggested change
setup(err => {
setup((err) => {


app.post(uriItems, prepareQuery, ensureContentType, options.preMiddleware, options.preCreate, accessMiddleware, ops.createObject, prepareOutput)
app.post(uriItem, util.deprecate(prepareQuery, 'express-restify-mongoose: in a future major version, the POST method to update resources will be removed. Use PATCH instead.'), ensureContentType, options.preMiddleware, options.findOneAndUpdate ? [] : filterAndFindById, options.preUpdate, accessMiddleware, ops.modifyObject, prepareOutput)
options.generateMethods.forEach(method => {
Copy link
Owner

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace method with (method)

Suggested change
options.generateMethods.forEach(method => {
options.generateMethods.forEach((method) => {

)
})

it('PUT /Customer 404', done => {
Copy link
Owner

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace done with (done)

Suggested change
it('PUT /Customer 404', done => {
it('PUT /Customer 404', (done) => {

})
})

after(done => {
Copy link
Owner

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Replace (done with ((done)

Suggested change
after(done => {
after((done) => {

@@ -19,7 +19,8 @@ function getDefaults() {
runValidators: false,
allowRegex: true,
private: [],
protected: []
protected: [],
generateMethods: ['delete', 'deleteone', 'get', 'getone', 'getcount', 'getshallow', 'patch', 'put', 'post', 'modifyobject']
Copy link
Owner

Choose a reason for hiding this comment

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

Codacy has a fix for the issue: Insert ,

Suggested change
generateMethods: ['delete', 'deleteone', 'get', 'getone', 'getcount', 'getshallow', 'patch', 'put', 'post', 'modifyobject']
generateMethods: ['delete', 'deleteone', 'get', 'getone', 'getcount', 'getshallow', 'patch', 'put', 'post', 'modifyobject'],

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants