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

ValidationService: Use joi for data validation #416

Closed
WoH opened this issue Aug 15, 2019 · 9 comments
Closed

ValidationService: Use joi for data validation #416

WoH opened this issue Aug 15, 2019 · 9 comments

Comments

@WoH
Copy link
Collaborator

WoH commented Aug 15, 2019

Since becoming more familiar with TSOA internals and therefore the ValidationService, I noticed there is a lot of custom validation logic involved.

Would it make sense to shift these responsibilities to another library and avoid duplicated efforts?

Maybe https://github.com/hapijs/joi is a good candidate to promote from dev to runtime dependency.

@lukeautry @dgreene1 @endor

@lukeautry
Copy link
Owner

lukeautry commented Aug 15, 2019

@WoH I realize I'm pretty absentee on this project right now. My main focus (outside of my actual job, lol) has been on a library that does what you're talking about, but in a more TypeScript-ish way and could serve as a drop in replacement for tsoa's existing validation. Once I actually release this library there will be more to talk about.

What I'd like to avoid is adding runtime dependencies in the generated routes.

@WoH
Copy link
Collaborator Author

WoH commented Aug 15, 2019

@lukeautry sounds exciting, looking forward to that 👍

E: I'll close this, for now, thanks for the quick response.

@HoldYourWaffle
Copy link
Contributor

So what's the actual outcome of this? Are we going to use luke's library when it's finished?
Personally I feel like runtime validation shouldn't be TSOA's concern at all, in my mind it's just a router (and swagger spec) generator.

In my own usecase I use a custom template with my own 'validator' that links a type name to the appropriate (external) JSON-schema validator. Perhaps a solution could be adding a separate custom template for a validator implementation? That way you don't have to redefine the route generator if you just want a different runtime validation library, and we could still provide defaults for libraries like joi or the "vanilla" kind that's included now.

@dgreene1
Copy link
Collaborator

dgreene1 commented Sep 1, 2019

Personally I feel like runtime validation shouldn't be TSOA's concern at all, in my mind it's just a router (and swagger spec) generator.

@HoldYourWaffle you’ve mentioned this a couple of times in PRs and issues (here being the most recent). Let’s be clear: tsoa has always done validation. Validation is one of the goals of tsoa. You can say that you don’t think it should concern itself with validation, but that is one of it’s “critical path” features. So every design decision we make needs to factor in both swagger generation and runtime validation.

For those who want to use a custom template to circumvent the default validation like you do, they will always be able to do that. But for the primary user of tsoa, they expect that the swagger documentation and the validation to work in the same manner. In other words, if the swagger doc describes that a query parameter is a date, then the runtime validation should throw if it’s not a date.

@HoldYourWaffle
Copy link
Contributor

You're focusing on the wrong thing here, I never said anything about "removing validation" or anything remotely like that, I just asked for the status on this and provided some input (including a possible solution) based on my own use-case.

The built-in validation was very basic last time I checked (which is arguably pretty long ago now) and I can't imagine that the majority of users have it as their sole "validation layer". I'm definitely not saying that doing that is "wrong" or anything like that; it's just that, at least in my experience, validation requirements can get much more complex than what the built-in validator can provide. I personally think that it shouldn't try to be an all-in solution for that either, but that's not what I wanted to discuss.

Yes, you can "remove" the built-in validator using custom templates, but that's not very convenient if you only want to customize validation. What I tried to propose was a system similar to custom router templates: a separate template for just the validator that can be freely mixed-and-matched with the router-template. We would of course still provide the default validator, and possibly even some additional ones like JOI (just like we do with router generation).

I'd love to hear some actual feedback on this idea instead of being accused of going against the design goals of the project, as in my own humble opinion, "TSOA has always done validation" is not an argument against a more flexible system for the same validation.

@dgreene1
Copy link
Collaborator

dgreene1 commented Sep 8, 2019

I think something might have gotten lost in translation, so I'd like to clarify why I specifically responded to the "all-in-one" aspect of tsoa.

But first, let's break that comment down for the moment:

"TSOA has always done validation" is not an argument against a more flexible system for the same validation.

I was not saying that the past is written in stone. To be 100% clear, I am not advocating for status-quo. I believe that software can and always should improve to support the needs of it's users. What was trying to communicate is that tsoa's main benefit is that it is an all-in-one library for validation and the documentation of that validation. We can not deviate from that main benefit because we would simply be duplicating the many libraries that only do one of the things (validation or documentation).

instead of being accused of going against the design goals of the project

No accusation is being made. However, you stated the following:

Personally I'd say that runtime validation shouldn't be TSOA's business at all because I see it as just an auto-generation library/framework

And then later you said:

I personally think that it shouldn't try to be an all-in solution for that either, but that's not what I wanted to discuss.

As I hope to clarify below, the topic of whether or not tsoa should be in the business of runtime validation is a moot point because that is precisely the advertised value it brings to the NodeJS ecosystem. For anyone who does not want an "all-in-one" solution, they can use one of the multitude of alternative options available on the market that only do one of the things (validation or documentation).

However, I sincerely hope that people choose to use tsoa for both the generation and for the validation because I believe there is incredible value in being clear and transparent about the acceptable inputs for an API. By being an "all-in-one" product, tsoa is capable of being completely transparent to the API consumer about what will constitute an error response. This transparency is why I love tsoa and why I jumped at the chance to take ownership of the npm package when Luke contacted me.

Getting to the original topic of this issue

However, since tsoa does two things it has some necessary bloat (although most of that bloat is stripped away at compile time). But for maintainers and contributors, it does appear (at first glance) to create some code that could be simplified by using an external library to do to one or both of the two things that tsoa does (swagger generation + swagger validation).

That is why I believe @WoH submitted this design suggestion: He wanted to know if we would consider using a library to do the validation part for us.

So when you say that you want a more flexible system for the same validation you're discussing a topic different than what @WoH is proposing. @WoH wanted something easier for maintainers/contributors of the library, and I believe that you are proposing more flexible validation for the users of the library.

So what's the actual outcome of this?

The outcome is that the creator of tsoa (Luke) said that he didn't want to bring in an external library. I (the current owner of tsoa) agree. I think it would be problematic to rely on an external library. However, I might be convinced once I see what Luke is working on. The creator of this issue (@WoH) thought that was a reasonable response and closed this issue. So the issue of using an external validator is closed for the time being.

We can now move onto your separate topic of "if you only want to customize validation."

So, how would users get more flexible validation

First off, I'm not sure why someone would want more flexible or powerful validation.

The built-in validation was very basic last time I checked (which is arguably pretty long ago now)

Before I became a tsoa maintainer, I was first a user of the library. At that time I was really impressed with the extensive validations and the flexible options that tsoa provides. So I would like to know more about what specific validations you can't accomplish with tsoa (before you answer, please consider that tsoa is all about "swagger compliant APIs" so it's validation should be limited to the shape of the data, not the business value of the data (which typically isn't documented in Swagger anyway since business issues are usually thrown as 422/UnprocessableEntity errors)).

and I can't imagine that the majority of users have it as their sole "validation layer".

I do, haha! Jokes aside, I was an enthusiastic user of tsoa before I became a maintainer. I was very impressed with the multitude of options that tsoa provides for validating the shape of the data.

But if someone does find something to be missing in tsoa's feature set, then I would love for those people to create an issue so we can add that functionality to tsoa.

On the topic of an external validator

I'd love to hear some actual feedback on this idea

My feedback is that it tsoa should not allow for custom validation. If an API wants to write custom validation, tsoa does not prevent them from doing so. You simply check the data inside of the tsoa route and throw a 400 error if the conditions you do not want have been found.

Since tsoa's first goal is TypeScript controllers and models as the single source of truth for your API I don't think tsoa should allow for a system similar to custom router templates: a separate template for just the validator that can be freely mixed-and-matched with the router-template. Here's why:

  • it breaks "Curly's Law" of "doing only one thing". Tsoa already breaks that by technically doing two things (validation and documenting that validation). So if tsoa were to also provide a framework for custom validation, that would extend the maintenance cost of tsoa.

Summary

  • If tsoa is lacking some typing validation capability, make an issue and we will discuss how we can include that type of validation.
    • of course we will only be able to support that validation within tsoa if:
      • swagger/OpenAPI has a way to communicate that validation
      • we can generate that validation/documentation via the design goals's described mechanisms (typescript types first, otherwise JSDocs)
    • If the above criteria can not be met with the proposed validation, then users have alternative options:
      • don't use tsoa and use a non all-in-one library so you are free to "roll-your-own" system
      • use tsoa, but just validate the incoming data in the controller method after tsoa's routes.ts file has done it's own validation

Action items:

  • @HoldYourWaffle if you would like to help me better understand your concerns, please clarify what validations tsoa is not currently capable of doing.

@HoldYourWaffle
Copy link
Contributor

Upfront disclaimer: I'm pretty tired right now, I tried as best as I could to explain myself but I probably didn't catch everything. Just say the word if you need me to clarify some more :)

I definitely think some things got lost in translation.
Just to be 100% clear: I'm not proposing to remove or otherwise change the current "default validator". My personal opinion is that TSOA shouldn't try to be a validator, precisely because of the Curly's Law that you already mentioned yourself. This is my opinion, and I understand and 100% respect your choice to not adopt it.

Now on to the actual discussion again (do you want me to create a separate issue for this?).
Last time I checked the validation was very basic, I think I only saw typeof comparisons (which is fine for primitives of course). This was a very long time ago though, so it's entirely possible that this has changed (or that I completely missed all more advanced features).

I currently use JSON-schema for my validation needs, most of the features I require are:

  • Minimum/maximum length for strings/arrays
  • Regex pattern matching
  • Enums
  • Minimum/maximum value for numbers
  • Required/optional properties

Some of those are very basic of course but I still included them because I have no idea if the default validator handles them already (I can't test it right now either).


However, even if TSOA were to support all these validation features (and others I might want to use in the future), I'd still be more comfortable with using a battle-tested dedicated validation library, since such a library has validation (and therefore security) as it's primary goal and purpose.

Another reason why I think TSOA shouldn't market itself as a validator is that it would be reinventing the wheel. There are already dozens of validation-oriented libraries out there, and a lot of them will probably be able to do a better job than TSOA ever could because they're specifically targeted towards validation (this is more prevalent for complicated validation scenario's).
I think we should keep the current 'default' validator, but I also think we should allow users to easily replace or extend it where it falls short.

TSOA is also bound by whatever Swagger/OpenAPI supports, and those sadly don't support every kind of validation. I don't see a reason for "alienating" users that might not care about these specs by making it hard for them to fill in for their shortcomings.


My feedback is that it tsoa should not allow for custom validation. If an API wants to write custom validation, tsoa does not prevent them from doing so. You simply check the data inside of the tsoa route and throw a 400 error if the conditions you do not want have been found.

I could validate the data manually inside the route, but doesn't that kind of defeat the point of automatic validation? Facilitating automatic validation (through code generation) is one of the two main reasons why I use TSOA, without it I'd basically be back to square one.

I'm currently automatically validating incoming data with a custom router generation template, which works absolutely fine but it requires me to use a custom template for everything. This is inconvenient if I only want to switch out my validator and still keep the default router generation.

It should also be noted that for this to work I needed to make some minor adjustments to the generation code, I'll see if I can create a PR for that sometime (it might not be 100% necessary).


Since tsoa's first goal is TypeScript controllers and models as the single source of truth for your API I don't think tsoa should allow for a system similar to custom router templates: a separate template for just the validator that can be freely mixed-and-matched with the router-template.

I don't see how those two things are related, could you clarify?


So if tsoa were to also provide a framework for custom validation, that would extend the maintenance cost of tsoa.

I think my proposal (I wouldn't call it a 'framework') would massively increase maintainability, at least in the long run.

  • First off, moving the validator to a separate file is nice for readability
  • Secondly, you allow users to bodge their own solution together if the one we provide is not sufficient (I'm aiming at more complex validation requirements here)
  • And finally it reduces duplication by having just one separate template that's injected into the selected router type, instead of three separate but identical (or at least very similar) blobs for each one.

I didn't consider the 'framework' argument when I thought of this, although I wouldn't call it a framework. We'd of course have to "formalize" some kind of interface by which the router templates can communicate with the validator templates, but this shouldn't be too difficult or time consuming. If I'm not mistaken the only difference with how things are done now would be cross-router-type parameter retrieval, but I haven't really looked into this yet.

To summarize: it's definitely not a trivial, oh-I-have-a-few-spare-minutes improvement, but I think it will massively help maintainability and developer experience in both the short and long term.

@dgreene1
Copy link
Collaborator

Every validation you asked for is already in tsoa.

@HoldYourWaffle
Copy link
Contributor

Not sure if this was all added recently or if I was just blind, but I have now found the actual validation 'implementation' so to speak. I'm pretty sure I rummaged around in those files before when working of #445 and #451, no idea how I didn't manage to connect the dots.

You didn't address my proposal though. Even though TSOA probably has all the features that I need right now, there's still a good chance that I may want to extend it in the future (for example by using JSON-schema).
There's also still the "trust issue", I don't really feel comfortable entrusting TSOA instead of a widespread, battle tested and dedicated solutions like JOI or AJV with my validation needs.

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

No branches or pull requests

4 participants