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

[BUG] ResponseFilter does not serialize #2555

Closed
ktalebian opened this issue Dec 22, 2023 · 4 comments
Closed

[BUG] ResponseFilter does not serialize #2555

ktalebian opened this issue Dec 22, 2023 · 4 comments
Assignees
Labels

Comments

@ktalebian
Copy link
Contributor

ktalebian commented Dec 22, 2023

Describe the bug

I use Generics and CollectionOf for a pagination response type. This is working perfectly, and everything is serialized without an issue.

For a particular use case, I decided to use https://tsed.io/docs/response-filter.html - this filter is to conditionally modify certain response objects. The response type is not changing in the modification - just some of the parameters are modified:

@ResponseFilter('application/json')
export class PaginationResponseFilter implements ResponseFilterMethods {
    transform(data: unknown, $ctx: Context) {
        if (!($ctx.data instanceof Page)) {
          return data;
        }
    
        if ($ctx.event.request.url) {
            $ctx.response.status(206);
            return $ctx.data.toResponse($ctx.event.request.url);
        }
    
        return data;
    }
}

Where toResposne object would modify some fields, and ultimately return another Page:

public toResponse(url: string): Page<T> {
    // ultimately return a new version of Page
    return new Page(this.list, this.metadata);
  }

Without response filter, I get the following response:

❯ curl localhost:8083/hosts | jq .
{
  "list": [
    {
      "id": "0396b829-5414-4519-9387-c817e2915715",
      "name": "host1"
    },
    {
      "id": "deed0453-6146-4a96-a4d9-a0c468b6e005",
      "name": "host2"
    }
  ],...
}

With the response filter, I get:

❯ curl localhost:8083/hosts | jq .
{
  "list": [
    {
      "id": {
        "value": "0396b829-5414-4519-9387-c817e2915715"
      },
      "name": "host1"
    },
    {
      "id": {
        "value": "deed0453-6146-4a96-a4d9-a0c468b6e005"
      },
      "name": "host2"
    }
  ],...
}

Note how the id field is {"value": "..."} vs being the actual value.

Here is a repo you can run: https://github.com/ktalebian/tsed-bug

To Reproduce

Clone https://github.com/ktalebian/tsed-bug and run

Expected behavior

I expect both responses to be identical

Code snippets

No response

Repository URL example

https://github.com/ktalebian/tsed-bug

OS

macOs

Node version

Node v18

Library version

v7.53.0

Additional context

No response

@Romakita
Copy link
Collaborator

ok, the serialization is run before the response filter.

  async flush($ctx: PlatformContext) {
    const {response} = $ctx;

    if (!$ctx.isDone()) {
      let data = await this.responseFilter.serialize($ctx.data, $ctx as any);
      data = await this.responseFilter.transform(data, $ctx as any);
      response.body(data);
    }
  }

https://github.com/tsedio/tsed/blob/production/packages/platform/common/src/services/PlatformHandler.ts

your code is wrong because it work on the $ctx.data and not over the data itself. it's not a bug you choose to work on the $ctx.data.

I should even make a fix to prevent you from using $ctx.data, as the devs don't do bad implementations in the response filter ^^.

Two solution:

  1. make transformation over data, just use $ctx.data to dermine if it's a Page instance, but apply url on data
  2. Inject PlatformResponseFilter and call serialize method, but, it's much less efficient to do that.

I close this issue
See you

Copy link

🎉 Are you happy?

If you appreciated the support, know that it is free and is carried out on personal time ;)

A support, even a little bit makes a difference for me and continues to bring you answers!

github opencollective

@Romakita
Copy link
Collaborator

I'll update the pagination to clarify this issue ;)

@Romakita
Copy link
Collaborator

#2556

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

No branches or pull requests

2 participants