New package: mdg:validated-method - Meteor methods with better scoping, argument checking, and good defaults

announce
14

You’re missing displaying helpful errors if the method is called from user input - so for example if the user fills out a form, they would want to know that their input failed because a certain field is too long.

I’m not sure that the client knowing what type a certain argument is supposed to be can be considered a security vulnerability - in some sense, the API between your client and the server can’t be a point of security since anyone can de-minify your client code and simply read how your methods are called.

Although if there is a good reason to hide the errors in production we could add an option to hide errors from the client.

15

Yes, the form example is a good one. So how would that work? Is there a way on the client to capture the method validation errors so that they could be displayed? Or is that a different pattern?

16

Didn’t I read a quote somewhere where the author of simple-schema supposedly said that if he could go back in time, he would use Astronomy? :smile:

At least I took that as a hint to start trying that instead of simple-schema…

2 Likes
17

Thanks for taking the time to present a clearer/broader picture.

Let me first reiterate that I am assuming this is a piece of work that’s going to lose the mdg: prefix and end up part of core, and I’m building my arguments upon this assumption.

As we’ve seen with the Blaze discussion, a major/vocal percentage of the community does not like it when something is standardized and then deprecated. And that such decisions should not be spur of the moment things, well thought out, planned and executed for long term sustainability.

Whatever incremental step we take here, it has to be supported in the long term and it would also pose as a stance. The even more monumental undertaking would be “hey ss is great and we all asked you to switch to it but you know, xxx is now the js standard so everyone’s to switch again”.

If you really want to improve on and push SS as a javascript standard (I wish MDG had the same enthusiasm for Blaze) please first propese a draft plan on how you’d like to achieve it. How we could help. How realistic it is from the broader JS perspective.

So if you are going to work with @aldeed, please do so, and in fact make that an official and public statement. Don’t depend on “possible future PRs” which you or aldeed may or may not find the time to look into. You say MDG may not be fast enough to accepting PRs. Well, who guarantees the opposite for aldeed?

I’m all for the well intended push forward, but Meteor is now 1.x and stability and long term support are important aspects of the platform.

Perhaps you may want to check https://atmospherejs.com/bshamblen/json-simple-schema and https://atmospherejs.com/fullflavedave/json-schema out.

My remark about not embracing the ecosystem is about shutting out SS alternatives. So if one likes the design decisions of astronomy or wants to go with json schema, it would just mean doubling the efforts by repeating code that already does the same as SS within methods. Noone wants to write the same code twice. Hence the end of SS alternatives, the “not embracing” remark.

So I guess, I do support the two options you mention, go with SS and make it a standard, or search for the standard, or the closest thing to a standard. The problem is, you have already chosen, so there actually is no choice left here.

Anyway, I still love that you took the initiative and time to fix methods and by the looks of it, it is going to end up a huge improvement over its current form.

18

See also meteor-call-sync.
This should have been made the default Meteor behavior long ago, IMO.

19

We did actually spend some time looking around and calling for suggestions: https://github.com/meteor/guide/issues/53

JSON Schema seems to be the closest thing to a “standard” right now, but I think it’s a long way away from being that, TBH. I don’t think it really achieves what we need it to, in terms of features, and SS is very much part of the Meteor community already, so blessing it is way less impactful.

I’m not sure how having lots of options out there without any clue of which one you “should” use helps with code duplication though? I mean, if we’d been quicker to embrace SS, then I would guess Astronomy (which does a lot more than just schema validation) would probably have used SS for its schemas, and we’d be in a better place right now (from a duplication point of view).

(Part of the idea of this work is to make a common format for validation errors that isn’t schema specific – please open issues / PRs against https://github.com/meteor/validation-error for this!)

Subclassing mdg:method to take a different type of schema and do the same thing that we do with SimpleSchema (throwing a ValidationError) seems pretty trivial – so I don’t think we are “shutting out alternatives”, except if you mean choosing something to recommend in the guide and elsewhere; I think the consensus is that it’s a very good idea for us to do this.

20

What I said is that I wouldn’t have had to create simple-schema/collection2 if astronomy existed back then. I did create it, though, and tons of people rely on it and so do many of my production apps, so I have no plans to stop supporting it until people stop using it. I’m working on v2 now, part of which will be speed improvements and moving as much of it as possible to a pure node package.

15 Likes
21

To be clear, are you saying that from hence-forth I must use simple-schema to build Meteor apps? Or are you saying this is just considered best-practices by MDG?

22

Thoughtful, and I like the spirit of it generally! Still, Promises are part of core now, and they’re not used, that’s my only critique. Can you omit the callback parameter and get a Promise ? If not, is there any guarantee of whether the callback is invoked sync or async, or multiple times, for the simulation, and the eventual real ?

2 Likes
23

Ah I was hoping you would comment! What do you think is the best way to integrate promise support here? I feel like it’s not yet a good time to switch to always returning a promise, and doing it only sometimes seems weird. Some options:

  1. Add support for mixins, have a promise mixin that wraps call
  2. Make a subclass class PromiseMethod extends Method that wraps call
  3. Add a new method callAsync or callPromise

As for the callback, it has the same semantics as Meteor.call - is there some case where that invokes the callback twice, or synchronously? This wrapper could be a good place to fix that, if so.

24

You don’t need to use simple-schema, but that will be the path we recommend. The idea is that the core Meteor framework can remain flexible (and actually become more flexible) and the guide project can be completely opinionated.

2 Likes
25

I’m not sure if you saw this https://github.com/meteor/meteor/pull/5005 but my impression was that making methods return promises is a non-trivial exercise for some complicated reasons…

26

You’re making a good point by mentioning Astronomy here. I think that Meteor is definitely lacking a robust and standardized way to set up model classes and hook them to the underlying collections (including validation) in a DRY way. Something that BaseModel and Astronomy are trying to achieve.

I’m using neither of them at the moment (because I did not have enough time to rework my code), so I know the pain that arises from not having such a model wrapper. If I change a field in one of my model classes, I also have to update my SimpleSchema for the collection (which I am not forced to use, but helped me a lot to detect issues), and this of course is double work.

So, instead of just hooking SS into this new way of method invocation, wouldn’t it be better to introduce a more object-oriented modeling paradigm to Meteor, by which the method invocation itself could then decide which validation to use, based on the types of the input parameters? I am speaking of a way of introducing stronger typing to Meteor.

To keep things simple for beginners, this strong typing mechanism could be made opt-in (just like autopublish), but for a maintainable app it would be a must.

1 Like
27

We did actually spend some time looking around and calling for suggestions: https://github.com/meteor/guide/issues/534

Looking around (with only one more person other than you and aldeed) is one thing, bringing this up with the community, like you guys did on the Blaze topic is another thing. Besides, it was a “guide” issue, not a “core” issue that revolved around redefining how methods work.

JSON Schema seems to be the closest thing to a “standard” right now, but I think it’s a long way away from being that

So yeah, instead of embracing and contributing to the standards, let’s do something else (Blaze?) and then when the broader JS community finally comes up with a standard, let’s ditch all the hard work and the effort?

f we’d been quicker to embrace SS, then I would guess Astronomy … would probably have used SS for its schemas, and we’d be in a better place right now

Well, it’s not what happened and you really can’t say that on behalf of Astronomy’s author. In fact aldeed himself said the exact opposite, that he would have not bothered creating SS had astronomy existed back then. But that’s besides the point. I’m not advocating astronomy here.

I don’t think we are “shutting out alternatives”, except if you mean choosing something to recommend in the guide and elsewhere

Again, are we talking about the guide or a package that’s very likely to become core? For the latter, taking in SS is not recommending anything, it is a bold statement for SS and against anything else. Subclassing etc are then just technical politics and far cries of the remaining alternatives.

I think the consensus is that it’s a very good idea for us to do this.

Again, whose consensus? From the github issue you’ve linked, it looks like two or three people.

So my overall point is, I think this decision of yours is a statement of how MDG operates and how Meteor and the community is governed. And it just does not help heal the many similar catastrophies we’ve had until now.

28

I’d say every time since its important to know how it works and what problems it solves. It can be like a pre-requisite. Maybe the package could be offered in the end as a shortcut “now you know, let’s save some time” - but I’m not comfortable with this being a core package.

1 Like
29

This is a big improvement of methods, and I would happily switch over to use the package. I understand @serkandurusoy though. Playing with the package is one thing, but using it in business apps is another with real money at stake.
It would help to know if it will become part of core or being maintained in the long run, so @sashko or @tmeasday maybe can tell something on their plans or long term commitment?

30

@aldeed after all this blaze drama, news of simpleschema v2 node package is a surprising ray of sunshine!!!

2 Likes
31

Yeah, now you will have to rewrite your schemas too! :smile:

3 Likes
32

So if check goes by the wayside in favor of Simple Schema, should be then possibly develop a pattern for using it in our publications as well?

2 Likes
33

An interim would be to use check(input, schema) since SS already supports use as a match pattern.

1 Like