amplication / amplication

🔥🔥🔥 The Only Production-Ready AI-Powered Backend Code Generation
https://amplication.com
Other
15.16k stars 1.48k forks source link

Create many - Route #4234

Open vincenzodomina opened 1 year ago

vincenzodomina commented 1 year ago

Feature description

Additionally to the "Create one" Route I suggest to have an/ or change it to an "Create many" Route as the standard POST Route in the REST-API

Use case

Problem: Instead of having to loop on the client side to create many records from an array of items and thus having many http-calls that are slow or even fail with an ERR_INSUFFICIENT_RESOURCES in the browser, the standard POST Route could be a "Create Many" Route

The feature could look like this in the UI (like on the Permissions page) i can select between an "Create one" or "Create many" Route for a given Entity: image

Are you willing to submit PR?

Yes I am willing to submit a PR!

souravjain540 commented 1 year ago

Thanks for opening the issue.

@abrl91 please validate and he will make a PR

abrl91 commented 1 year ago

Thanks for opening the issue.

@abrl91 please validate and he will make a PR

approved

souravjain540 commented 1 year ago

@vincenzodomina assigned it to you. Let us know if any help needed

vincenzodomina commented 1 year ago

@souravjain540 I would need help with the decision if we should replace the POST route with an array of entities as request body (instead of just one as currently), or if this behaviour should be made selectable as option in the UI.

Sorry, I posted two feature approaches.

Unfortunately Prisma "createMany" cannot handle relations in the way "create" can so i will try to loop through in the controller and use "create".

souravjain540 commented 1 year ago

@souravjain540 I would need help with the decision if we should replace the POST route with an array of entities as request body (instead of just one as currently), or if this behaviour should be made selectable as option in the UI.

Sorry, I posted two feature approaches.

Unfortunately Prisma "createMany" cannot handle relations in the way "create" can so i will try to loop through in the controller and use "create".

@abrl91 , @arielweinberger please answer

vincenzodomina commented 1 year ago

My code changes are creating a lot diff compared to the test snapshots. Is there a way to update the test snapshots (to not have to touch the files manually for every commit) ?

Edit: Google just magically answered it for me, just had to run npm test -- -u Maybe worth as addition in the readme for people that haven't worked with jest before.

abrl91 commented 1 year ago

My code changes are creating a lot diff compared to the test snapshots. Is there a way to update the test snapshots (to not have to touch the files manually for every commit) ?

Edit: Google just magically answered it for me, just had to run npm test -- -u Maybe worth as addition in the readme for people that haven't worked with jest before.

In the package.json file of amplication-data-service-generation you have these two scripts:

    "generate-test-data-service": "ts-node scripts/generate-test-data-service.ts generated",
    "update-test-data-service-snapshot": "jest -u src/tests/*.spec.ts && jest -u src/tests/server"

The first one generates an app inside packages/amplication-data-service-generator/generated folder. The second runs the snapshot tests, and when it ends, you can see the diff in the code (you will see in the files changed snapshot files).

yuval-hazaz commented 1 year ago

@GreenMachine01 @arielweinberger we need to make a product decision on if and how we want to implement this fix. @vincenzodomina you provided very valuable feedback, and I agree with the need, but we need to make sure we implement a fix in a way that will be maintainable and will not break existing projects.

GreenMachine01 commented 1 year ago

@arielweinberger what is in the standards that we want to follow for the POST Route

  1. Create One Route
  2. Create Many Routes
arielweinberger commented 1 year ago

Hey @vincenzodomina, sorry for not getting back to you earlier!

Looking through this proposal, I clearly understand the need. Sometimes it's trivial to create multiple instances of a resource.

I think being too opinionated about whether the default POST /resource endpoint should support a single entity (object) or multiple (an array) is a no-no. I would prefer to keep REST endpoints simple and good at one thing.

This is commonly solved by introducing a /batch endpoint. For example, to create multiple orders, that would be POST /order/batch, which works the same behind the scenes, but accepts an array instead.

I have no doubt that this feature will be useful to our users at one point or another. Here are some suggestions:

Since this feature has not been requested before, I am not sure if it would be wise to dedicate our internal team's time to it until there is more demand and reasoning. However, since I see you are happy to pick this up, I am happy to let you do that and support you!

What do you think? Do you have any feedback about my suggestions above?

vincenzodomina commented 1 year ago

Thanks @arielweinberger for your feedback and solution! good idea!

With my proposed "multiple entity support"- solution for the default POST /resource endpoint you also can just pass a single item in the array and it would create only a single record. I like the response code 207, that can also be applied to both solutions.

Personally I think having only one route with expecting an array is cleaner as it offers both (Create one and create many) and you only have one route instead of two, but this is only personal preference and both solution work just fine.

The advantage of your proposed solution, it is not a breaking change.

arielweinberger commented 1 year ago

Personally I think having only one route with expecting an array is cleaner as it offers both (Create one and create many) and you only have one route instead of two

While I see your point of view, I think this is deviating from common practices of REST APIs, by compromising predictability and consistency. For example, clients will have to be aware of whether they send an array or a single object, to act on the different response type accordingly. This also makes type safety and type definitions more complex which I would like to avoid. In this particular case, I don't think we gain value from minimizing the number of endpoints. This being a breaking change is of course a consideration as well, as you said.

If you'd willing to pick this up, you've got my blessing and support! Should I assign it to you?

vincenzodomina commented 1 year ago

Thanks for this input!

Please help me to understand this better:

For example, clients will have to be aware of whether they send an array or a single object, to act on the different response type accordingly.

In my proposed solution here I don't offer both possibilities (single object, array of object) but always only an array of objects. So if a user only wants to create one record he would send only one object inside the array. So it is always an array of objects per type definition. So it is predictable and consistent right?

yuval-hazaz commented 1 year ago

The original PR was closed with the following comments:

|after additional consideration and research, we've decided that bulk creation should be implemented with a separate endpoint.

The restful standard talks about the POST for creating a single resource. There is a de facto standard of using /resource/bulk to create multiple resources.

See this for example https://woocommerce.github.io/woocommerce-rest-api-docs/#batch-update-products

Implementing it this way will also not break existing apps and will give the developer more control over the generated code for edge use cases.

So, what I suggest is:

Create a new endpoint on the controller for /bulk Move the iteration logic into the entity service and expose a single function CreateBulk(Entity[]) We also need to return detailed error codes for each resource in case of failure If you want to implement that, we would be happy to help with it. If you prefer not to, we will create a new issue on the backlog and will implement it in one of the future releases.

Note: This behavior should also be implemented in GraphQL with a mutation like "bulkCreateCustomer"