dbos-inc / dbos-transact

The TypeScript framework for backends that scale
https://docs.dbos.dev
MIT License
291 stars 20 forks source link

implement PUT/PATCH/DELETE handlers #484

Closed demetris-manikas closed 1 month ago

demetris-manikas commented 1 month ago

This PR adds support for PUT PATCH and DELETE http methods. I have not yet implemented any tests. I will go about writing them later in the week If possible it would be nice if someone from the team provided stubs of the required tests

demetris-manikas commented 1 month ago

Added support for PATCH as requested. The delete method with body arg is failing as well as the get with body arg that I added since it was missing. The only related content for the error that I managed to locate is https://github.com/ladjs/supertest/issues/399 Advise...

kraftp commented 1 month ago

Thanks Demetris! @chuck-dbos @qianl15 you originally implemented the HTTP argument parsing--can you take a look? Thanks!

qianl15 commented 1 month ago

I think the issue is that Koa body parser by default only parses POST, PUT, PATCH: https://github.com/koajs/koa-body/blob/v4.1.1/index.js#L72

As mentioned in this issue (https://github.com/koajs/koa-body/issues/163) a workaround is to add the parsedMethods option when we use the bodyParser middleware (https://github.com/dbos-inc/dbos-transact/blob/main/src/httpServer/server.ts#L206).

I think we should add DELETE to the list of parsedMethods but not GET, and remove the test for get with body arg. Because as specified here: https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/GET

Sending body/payload in a GET request may cause some existing implementations to reject the request — while not prohibited by the specification, the semantics are undefined. It is better to just avoid sending payloads in GET requests.

demetris-manikas commented 1 month ago

@qianl15 On the money! All test pass.

I guess that a search will probably reveal a similar advisory for not sending body data with the DELETE command as well.

I have been in the situation where the query was that long that I had to use body params on a GET.

Anyway a user may well define

@KoaBodyParser(bodyParser({
    extendTypes: {
      json: ["application/json", "application/custom-content-type"],
    },
    encoding: "utf-8",
    parsedMethods: ['POST', 'PUT', 'PATCH', 'GET', 'DELETE']
  }))

so why not have it tested?

demetris-manikas commented 1 month ago

What about the rest http commands, HEAD, OPTIONS, CONNECT and TRACE?

maxdml commented 1 month ago

What about the rest http commands, HEAD, OPTIONS, CONNECT and TRACE?

They're not as common, we could add them to another PR

chuck-dbos commented 1 month ago

What about the rest http commands, HEAD, OPTIONS, CONNECT and TRACE?

They're not as common, we could add them to another PR

HEAD is common. If we do anything with OPTIONS we have to make sure it doesn't interfere with CORS handling of that.

demetris-manikas commented 1 month ago

Padding increased to 6