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

getRolesForUser - "Cannot read property '_id' of undefined" on client after edit #308

Closed
cormip opened this issue Dec 16, 2019 · 6 comments

Comments

@cormip
Copy link

cormip commented Dec 16, 2019

I have a client-side "users" table that reactively displays the Meteor User accounts (name, email) and their roles. My template calls Roles.getRolesForUser() to retrieve each user's roles. Everything works fine on initial load.
However, when a user's roles are updated, i.e. Roles.setUserRoles(user._id, roles), via a server method, I get a console error on the client when the table reactively updates itself:

Exception in template helper: TypeError: Cannot read property '_id' of undefined
    at http://localhost:3000/packages/alanning_roles.js?hash=b56e6b2c0ee0a6e1aa6095edd24e7d0cf4e47bc1:875:132
    at Array.map (<anonymous>)
    at Object.getRolesForUser (http://localhost:3000/packages/alanning_roles.js?hash=b56e6b2c0ee0a6e1aa6095edd24e7d0cf4e47bc1:875:121)
    at fn (/imports/ui/pages/authenticated/admin/users.js:117:31)
    at Object.getField (http://localhost:3000/packages/aslagle_reactive-table.js?hash=d5959cc90cbe650f00f5b58fa8d77953153cecee:1223:16)
    at http://localhost:3000/packages/blaze.js?hash=51f4a3bdae106610ee48d8eff291f3628713d847:3051:16
    at http://localhost:3000/packages/blaze.js?hash=51f4a3bdae106610ee48d8eff291f3628713d847:1715:16
    at http://localhost:3000/packages/blaze.js?hash=51f4a3bdae106610ee48d8eff291f3628713d847:3103:66
    at Function.Template._withTemplateInstanceFunc (http://localhost:3000/packages/blaze.js?hash=51f4a3bdae106610ee48d8eff291f3628713d847:3769:14)
    at http://localhost:3000/packages/blaze.js?hash=51f4a3bdae106610ee48d8eff291f3628713d847:3102:27

I am also null-publishing the users' roles:

Meteor.publish(null, function () {
    if (this.userId) {
        return Meteor.roleAssignment.find({ 'user._id': this.userId });
    } else {
        this.ready()
    }
})

Any thoughts on how to address this? It seems like it may be a race condition where the user's roles are removed (and then added) while Roles.getRolesForUser()is running?

@alanning
Copy link
Contributor

If you have a reproduction you could link to that would help.

Otherwise, could you post the "readable" javascript around where the exception is thrown, please?

@cormip
Copy link
Author

cormip commented Dec 16, 2019

The line that's crashing in roles_common.js is line 780:

return [...new Set(roles.map(r => r.inheritedRoles || [r.role]).reduce((rev, current) => rev.concat(current), []).map(r => r._id))]

roles = [] when that line of code is run, resulting in the crash.

However, the code automatically runs a second time with the roles array populated with the role assignment, which allows the code to work fine and the page to render correctly.

I think the app is reacting to the old roles being deleted by setUserRoles, resulting in roles = [] when querying Meteor.roleAssignment, then again reactively updating when the new roles are set by setUserRoles? Just to be clear, I'm only calling setUserRoles once with the new role assignments.

@SimonSimCity
Copy link
Member

I've myself tested it here, and it only occurs when having a role-assignment without role - which is pretty weird.

Can you please confirm this - and also (in case you have it) look at a place this could come from?

A sandbox application, where I could try this out would be nice.

@Shelagh-Lewins
Copy link

I've experienced the same problem, but only when my app is open in two different tabs or browsers. And if it's two tabs in the same browser, then the order of the tabs makes a difference. No, really! I tested it like 15 times!

I am using observeChanges to dynamically track user roles based on whether their email address is verified:

	Meteor.users.find().observeChanges({
		'changed': function (_id) {
			const user = Meteor.users.findOne({ _id });
			if (!user) {
				return;
			}

			try {
				if (user.emails[0].verified) {
					Roles.addUsersToRoles(_id, ['verified']);
				} else {
					Roles.removeUsersFromRoles(_id, ['verified']);
				}
			} catch (err) {
				console.log(`error checking roles for user ${_id}`);
			}
		},
	});
});

The 'verified' role has been created in advance and works fine. If I run my app in a single tab, everything works. I verify an email address in one browser tab, while another is open on the app, getRolesForUser throws the error. I also saw the error appear in Firefox when I verified an email address in Chrome.

I am currently working around this by wrapping getRolesForUser in a try...catch block.

If I only have one browser / tab open on the app, there is no problem. I assume that there is some timing issue whereby addUserToRoles does not operate atomically but instead performs two database operations and a different threaded client can stumble in the gap.

@SimonSimCity
Copy link
Member

@Shelagh-Lewins or @cormip would you have time to write a sample sandbox for this?

@cormip you describe it as "running in a server method". What do you mean by this? Is it in a Meteor method? If yes, are you sure this code is not run by the client first - see https://guide.meteor.com/methods.html#call-lifecycle?

@Shelagh-Lewins For your code it looks quite straight forward to be only available on the server.

If you yourself want to dive in, here's the only place I add a role to the system: https://github.com/Meteor-Community-Packages/meteor-roles/blob/master/roles/roles_common.js#L478

The failing code (https://github.com/Meteor-Community-Packages/meteor-roles/blob/master/roles/roles_common.js#L780 [...new Set(roles.map(r => r.inheritedRoles || [r.role]).reduce((rev, current) => rev.concat(current), []).map(r => r._id))] only crashes if the array roles has an element which is not an object having a property role.

I suppose it's a race condition, which are usually hard to reproduce. Creating a sandbox project would be a great time saver to us because we don't have to spend time creating an environment where this might fail.

Just create a new Meteor project, make sure you have all the necessary components to reproduce this bug and upload it somewhere we can access it - e.g. to github.

@SimonSimCity
Copy link
Member

This issue should've been fixed in the just published version v3.2.1. Please request a reopen if the bug still persists.

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

No branches or pull requests

4 participants