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

args are not validated before being passed to subscribe #535

Closed
backbone87 opened this issue Feb 10, 2020 · 4 comments
Closed

args are not validated before being passed to subscribe #535

backbone87 opened this issue Feb 10, 2020 · 4 comments
Labels
Community 👨‍👧 Something initiated by a community Duplicate 🔑 This issue or pull request already exists

Comments

@backbone87
Copy link

backbone87 commented Feb 10, 2020

Describe the bug
Args are not validated before being passed to subscribe

To Reproduce

@ArgsType()
class MyArgs {
  @Field(() => String)
  @MinLength(1)
  public name!: string;
}

@Resolver()
export default class MySubscriptions {
  @Subscription(() => String, {
    subscribe: (root, { name }: MyArgs) => {
      return { next: async () => ({ done: true }) };
    },
  })
  public observe(@Args(() => MyArgs ) args: MyArgs): string {
    return args.name;
  }
}

-> Subscribe with name set to empty string
-> No error (error would occur on first resolver call, but we have empty observable, also it should not (re)validate on resolver call)

Expected behavior
Arguments being validated before being passed to subscribe

Enviorment (please complete the following information):

  • OS: Windows
  • Node: 12
  • Package version: 0.17.6
  • TypeScript version: 3.7
@MichalLytek
Copy link
Owner

Kinda duplicate of #200 and #175 👀

Middlewares stack was created before subscriptions and their dynamic topic or custom subscribe feature, so middlewares, validation and authorization happens for the resolve part of subscription, not before subscribing.

Custom subscribe means "escape from framework and do it by my own". You need to check the args by yourself.

I will leave it open for visibility purposes.

@MichalLytek MichalLytek added Bug 🐛 Something isn't working Community 👨‍👧 Something initiated by a community Duplicate 🔑 This issue or pull request already exists labels Feb 10, 2020
@backbone87
Copy link
Author

is it possible (= pretty straightforward) to extend/change the functionality? i would try to create a PR then

@MichalLytek
Copy link
Owner

I doubt that, it's too complicated and too big breaking change I think.

The whole stack has to be redesigned and refactored to make it work properly and be customizable. I will do that in near future.

@MichalLytek
Copy link
Owner

Closing as it will be covered in #200 and #542 🔒

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Duplicate 🔑 This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants