trailsjs / trails

:evergreen_tree: Modern Web Application Framework for Node.js.
http://trailsjs.io
Other
1.66k stars 70 forks source link

Footprint delete http request on multiple items #189

Closed jaumard closed 8 years ago

jaumard commented 8 years ago

Issue Description

As I see under all trailpacks, to remove multiple items you need to call url like DELETE http://localhost:3000/api/v1/mymodel?name=test

This will remove all items with name = test with works fine. But if you try something like : DELETE http://localhost:3000/api/v1/mymodel?isOk=true DELETE http://localhost:3000/api/v1/mymodel?nbTodo=4

None of this url will work because when params are parse they are all in string so on Footprint Controller and FootprintService we have isOk = "true" and nbTodo="4" and the ORM will crash or worse execute the query successfully but 0 items was deleted.

Environment

tjwebb commented 8 years ago

when params are parse they are all in string so on Footprint Controller and FootprintService we have isOk = "true" and nbTodo="4"

Does this happen for every orm? With waterline, for instance, I believe it checks the schema and always tries to typecast the values to the correct type. This will be hard to do in general, because without knowing the expected type (as defined in the ORM), it's impossible to say whether true should be a Boolean or a String.

jaumard commented 8 years ago

It's occurred when I use sequelize I will make some tests with Waterline too, if it's works with Waterline I suppose I will have to checks the schema and always tries to typecast the values to the correct type manually :/

jaumard commented 8 years ago

@tjwebb I just test with Waterline and same result it doesn't parse fields so items are not deleted...

tjwebb commented 8 years ago

Weird, ok. I'll take a look. Unfortunately, this problem isn't solvable in general. We have to know the type in order to cast it properly. I'll check waterline to see if something is missing.

jaumard commented 8 years ago

I ask if the feature exist on sequelize. I think FootprintService can check criteria fields type with model schema and try to parse it with the correct type before making the request. Like this problem solved. It's not trivial but doable I think.

tjwebb commented 8 years ago

It's different each time, though. The schema for knex, for example, is not so easy to get at programmatically: https://github.com/trailsjs/trailpack-knex/blob/master/test/app.js#L14-L17

jaumard commented 8 years ago

I see... Another possible solution is to send data under the body instead of query. Like this nothing to do, just change FootprintController. Because I prefer send to the body rather than have a query that doesn't fully work. Sound like a big bug to me.

tjwebb commented 8 years ago

I think this actually ties somewhat into the "unified orm" discussion that still needs to be hashed out. I'll give you a heads up though: for Trails 2.0, I'll argue that we should use GraphQL (https://github.com/graphql/graphql-js) as the central data modeling language, similar to how we use Hapi route objects to glue together all the different web servers.

jaumard commented 8 years ago

Yes and no, the power of Trails is to use frameworks that the user already know and don't have to learn new things, that's why ORM models definitions must have to be native (only trailpack model definitions need a unified definition) because if we use GraphQL (witch I don't know at all ^^) maybe some native feature will be lost... I will not debate more as we have to focus first on the v1 ^^ but happy to discuss this when you want. Thanks for the heads up @tjwebb I will look GraphQL soon so ;)

So the quick fix I see here for the v1 is to use body request instead query.

tjwebb commented 8 years ago

Users will never be forced to use GraphQL, or any other particular ORM technology. Just like now, users don't have to know Hapi in order to use Express4, even though under the hood, express is initialized from Hapi-style route objects.

We can apply the same concepts to the data layer that we used for the webservers. In Trails 2, we'll leverage graphql as the "common language" that's implemented under the hood for the datastore trailpacks. The goal of graphql as a common language would be to give other trailpacks the ability to integrate and interact with the ORM in a common way that's not currently possible. This will by no means be required.

Existing trailpacks such as knex and waterline can continue defining their models according to their own specifications. This currently is similarly possible with the webserver trailpacks -- another webserver trailpack could define whatever route object format it wants to -- but with the downside of not being able to leverage the power of trailpack-router and others that rely on this format. I think this tradeoff is perfectly fine, and will be the same for the datastore layer.

tjwebb commented 8 years ago

@jaumard maybe we should start another issue and continue the discussion there.

jaumard commented 8 years ago

@tjwebb ho ok agree with this :) we can continue to talk ORM/Model here https://github.com/trailsjs/trails/issues/94 and Footprint delete here ^^