[SOLVED] Meteor 2.0: useTracker dependency leads to "maximum update depth exceeded"

[niklasdada]

Hey,

I use a custom hook build with useTracker and pass the query-selector and the options (to provide sorting functionality on table header click) as dependencies.

contactsHook.js

export const useContacts = (query = {}, options = {}) => useTracker(() => {
    const subscriptionHandle = Meteor.subscribe('contacts', query, options);
    const contacts = Contacts.find(query, options).fetch();
    return {contacts, isLoading: !subscriptionHandle.ready()};
}, [query, options]);

contactList.jsx

export ContactList(props)  => {

    const [sortSelector, setSortSelector] = useState({});
    const [sortState, setSortState] = useState({key: null, direction: null,});

    const {contacts, isLoading} = useContacts({}, {sort: sortSelector});

    if (isLoading) return <Loader/>;

    const sort = (columnKey) => {
        // sort direction logic depending on semantic ui react
        const newDirection = sortState.key !== columnKey ? 'ascending' : 
            sortState.direction === 'ascending' ? 'descending' : 'ascending';

        setSortState({key: columnKey, direction: newDirection});
        setSortSelector(columnKey, newDirection === 'descending' ? 1 : -1);
    };


    return <Table>
        <Table.Header>
            <Table.HeaderCell
                onClick={() => sort("firstName")}>
                First Name
            </Table.HeaderCell>
            {/*...more headerCells and tableCode using contacts...*/}
        </Table.Header>
        {/*...tableCode using contacts...*/}
    </Table>
}

That worked fine before the meteor update. After the update I get

react_devtools_backend.js:2430 Warning: Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or one of the dependencies changes on every render.

→ The app runs into a loop of rerenders and crashes

When I remove the query-selector and the options and only pass a empty are as dependency to the hook, the warning is gone, but obviously the sorting doesn’t work anymore because a change of the sortSelector does not trigger a rerun of the useTracker hook anymore.

I wonder if this is a design mistake and meteor 2.0 is just more strict about that or if it’s a bug…

Some ideas about that? Thanks!

PS: Besides that I really really love meteor 2.0 and it’s hot module replacement…this makes the development so much more agile

[aureooms]

Is it because the query and options object change refs? To avoid this problem, I systematically wrap object deps as follows: deps = [JSON.stringify(query), JSON.stringify(options)]. This works even when query or options can be undefined or null. Otherwise you could use useMemo to keep the refs intact.

EDIT: See for instance When to useMemo and useCallback

[niklasdada]

Hey,
thank you so much! That was definitely the reason for the exhausive rerenders!
The JSON.stringify- workaround does a great job! In fact, I would say that it’s the better solution for hooks, because hooks are meant to be reusable, which means that, with useMemo, one would have to to ensure that the deps are memorized at every component which uses the hook. In contracst to this wrapping the deps in JSON.stringify “protects” the hooks from rerender.

I’m curious: What would be the arguments against let useTracker use some sort of stringify internally? Is there a situation where it’s necessary to run a hook when the stringifyed version of an object is identical but the reference not?

[aureooms]

@niklasdada

The JSON.stringify - workaround does a great job! In fact, I would say that it’s the better solution for hooks, because hooks are meant to be reusable, which means that, with useMemo, one would have to to ensure that the deps are memorized at every component which uses the hook. In contracst to this wrapping the deps in JSON.stringify “protects” the hooks from rerender.

I agree.

@niklasdada

I’m curious: What would be the arguments against let useTracker use some sort of stringify internally? Is there a situation where it’s necessary to run a hook when the stringifyed version of an object is identical but the reference not?

1. It would not follow the same API as react hooks.
2. It needs a non-standard stringification approach. JSON.stringify does not handle all cases.

[aureooms]

PS: You could use EJSON in case JSON does not handle Mongo.ObjectID. EJSON should support everything that Meteor.subscribe supports. For Collection#find however, there is a potential problem with the transform option. But since Meteor.subscribe only works with EJSONable arguments, you would have a problem there too.

[macrozone]

JSON.stringify on query and options work, but I would argue that your hook is maybe too generic and you should consider to be more specific with the arguments.

As an example: if you’d use always useContacts({ companyId: "xxx" }), you might consider to make the first argument the companyId and maybe even make the name more specific: useCompanyContacts("xxx"). In this case, you have simpler argument to pass to the dependency-array of useTracker that defines whether your hook needs to rerun. Check your actual usages of useContact and try to find a more specifc api. In general, functions should be as specifc as possible in general. Generalize when you actually need to.

On a side note, thats also why i generally recommend to use typescript, because it makes think about the “signature” (args-types and return type) of a function more carefully.

But there is a more serious thing to consider in your example:

You not only allow every query to be passed to the local minimongo-collection, you also pass this query to the subscription. This way, a malicious user might be able to get contacts that they are not allowed to. Therefore you also should be very specific with the publication function itself. Similar to the example above with the hook, your publication functions might just take some simple arguments and creates the query in the function itself. That way its more easy to check whether your publication is safe.

I’m curious: What would be the arguments against let useTracker use some sort of stringify internally? Is there a situation where it’s necessary to run a hook when the stringifyed version of an object is identical but the reference not?

useTracker uses useEffect or similar under the hood, so we could ask the same question for useEffect. @aureooms already mentioned the problem with JSON.stringify when it e.g. contains functions.

Another problem is that just stringify everything might be slow and relying on it maybe leads to anti-patterns, i guess. In most cases primitive values in deps array of useTracker (or `useEffect) are enough to solve most problems. Dan Abramov also shared some thoughts about that https://twitter.com/dan_abramov/status/1104414272753487872

By the way, there is an interesting proposal that should bring records and tuples to JS: GitHub - tc39/proposal-record-tuple: ECMAScript proposal for the Record and Tuple value types. | Stage 2: it will change!

they would remove the need to JSON.stringify in this case, because they can be compared like primitives! This proposal might have a significant impact on JS and react in particular as it relies a lot on immutability and shallow comparison

[aureooms]

@macrozone

You not only allow every query to be passed to the local minimongo-collection, you also pass this query to the subscription. This way, a malicious user might be able to get contacts that they are not allowed to. Therefore you also should be very specific with the publication function itself. Similar to the example above with the hook, your publication functions might just take some simple arguments and creates the query in the function itself. That way its more easy to check whether your publication is safe.

I agree. If you still want to allow for flexibility you need to sanitize the query and options given by the user. For instance I am working on an app where the access rule is straightforward: each document has an owner field which is some userId, a user can only read/write the documents they own. To enforce this rule in a subscription, I add the constraint { owner: this.userId } as follows:

Meteor.publish('publicationId', function (query, options) {
    const selector = {
        ...query,
        owner: this.userId
    };
    return Collection.find(selector, options);
});

Note that there is no precedence in object literals: if a malicious user tried to pass the owner field it would get overwritten by the hardcoded this.userId. This implementation is perhaps too implicit and you may want to loudly fail instead when this kind of misuse happens.

More complicated rules can be implemented in a similar way. For instance, if certain fields are “secret” you would have to parse and patch/reject the options parameter. As the rules get more and more complicated it becomes simpler to be very specific about the purpose of a publication and be very strict with allowed selector/options as @macrozone suggests.

PS: With this implicit approach you are not protected in case the semantics of Collection#find arguments changes. I doubt a drastic change would be a problem since DB interaction would stop working anyway. However, maybe an additional server-only option field could be abused to access documents that would otherwise not be accessible. All DB interaction would still work, but now a malicious user would be able to use this new server-only option field, and bypass the rules that needed to be enforced. To avoid this particular event in the first place, you could restrict options to allow only {sort, skip, limit, fields} to be passed down Collection#find.

[aureooms]

PPS: Can someone confirm that the implicit $and at the root of the query document cannot be bypassed by the $query meta operator?

[rjdavid]

It is a bad habit using a query selector coming from the client. The “inputs” can come from the client but creating the selector itself must happen in the server.

Anybody can bring down your database with a bad selector (not just about security but unoptimized queries).

You are also putting the burden on future developers to know that any changes they make on the schema, they must be aware that the query selector can come from the client

[aureooms]

Thanks @rjdavid! I had completely overlooked DoS attacks enabled by unoptimized queries and the client/server decoupling that dedicated publications offer.

[aureooms]

@aureooms

I agree. If you still want to allow for flexibility you need to sanitize the query and options given by the user. For instance I am working on an app where the access rule is straightforward: each document has an owner field which is some userId , a user can only read/write the documents they own. To enforce this rule in a subscription, I add the constraint { owner: this.userId } as follows:

Meteor.publish('publicationId', function (query, options) {
    const selector = {
        ...query,
        owner: this.userId
    };
    return Collection.find(selector, options);
});

Note that there is no precedence in object literals: if a malicious user tried to pass the owner field it would get overwritten by the hardcoded this.userId . This implementation is perhaps too implicit and you may want to loudly fail instead when this kind of misuse happens.

@peterfkruger @macrozone @niklasdada @rjdavid While in the example above the 'owner' key of query correctly gets overwritten even when this.userId is undefined, the undefined value gets erased somewhere down the pipeline from Collection.find to the MongoDB process (I believe MongoDB simply ignores undefined values for selectors). At the bare minimum you need to set owner: this.userId ?? null but it would be better to have a more declarative authentication mechanism that branches out to this.ready() when the app call is not authenticated, as illustrated in the second example here.

So yeah, better safe than concise. I am sorry if anybody has been using the code above in production without modifications. Please change it.