jagi / meteor-astronomy

Model layer for Meteor
https://atmospherejs.com/jagi/astronomy
MIT License
604 stars 67 forks source link

calling save() in callback func - Fiber Issue / Meteor.bindEnvironment #120

Closed purplecones closed 9 years ago

purplecones commented 9 years ago

First off, Great package! Very easy to use and lots of cool features.

Ok, I'm grabbing some data from an external source and trying to insert it into my collection. I have a callback function that is executed when the data is ready. In it, I loop through the results, each time creating a new item, setting the appropriate fields, and then calling save().

This works fine if I define the array manually without a callback and loop through it, but when it is in a callback, I have issues. Specifically, the issue is : Error: Meteor code must always run within a Fiber. Try wrapping callbacks that you pass to non-Meteor libraries with Meteor.bindEnvironment.

Thanks!

purplecones commented 9 years ago

I have it working, however not with Astronomy. Essentially what I am doing is seeding my Users collection with an existing set of users. I thought I would use Astronomy classes for this but not sure if thats the best approach as there is a specific Meteor method for creating users Accounts.createUser(). So I switched to that, however I still had the Fiber issue from before. To resolve that I figured out correct way to wrap the callback with Meteor.bindEnvironement.

        connection.query(query, Meteor.bindEnvironment((err, rows, fields) => {
          if (err) {
                throw err;
            }

            for(let row of rows) {
                // let user = new AstroClass.User();
                // user.set({
                //  emails: [{
                //      address: row.email,
                //      verfied: false
                //  }],
                //  profile: {
                //      firstName: row.firstName,
                //      lastName: row.lastName
                //  }
                // });
                // user.save();
                let userId;
                userId = Accounts.createUser({
                    email: row.email,
                    profile: {
                        firstName: row.firstName,
                        lastName: row.lastName 
                    }
                });
                Roles.addUsersToRoles(userId, [row.role], Roles.GLOBAL_GROUP);
            }
        }));

The commented out code was the Astronomy version I had which still gives me the Fiber issue, however I decided not go that route with the Users collection since it it closely tied to the Accounts package.

Not sure if this is still considered an issue, feel free to close if not.

lukejagodzinski commented 9 years ago

Thanks! :-)

You can use Astronomy or built in Meteor method, both will work. It depends on your needs. If you are sure that data from other database is correct then you can use the createUser method.

The problem with bindEnvironment is not related with Astronomy but with the wrong usage of Meteor. Meteor code has always to run within fiber. You can uncomment Astronomy code and it should work. Of not maybe I've forgot to do something. But let me know if it works.

purplecones commented 9 years ago

Hi @jagi ,

When I run the above code with the astronomy code instead, I get the following exception:

Exception in callback of async function: MongoError: insertDocument :: caused by :: 11000 E11000 duplicate key error index: meteor.users.$username_1  dup key: { : null }

Works fine when using Accounts.createUser(). Am I forgetting to set something?

lukejagodzinski commented 9 years ago

A user with the same name already exists so it's why you are getting this error. In Accounts collection there is an index set for the username field.

purplecones commented 9 years ago

The array has unique emails and it also works when using Accounts.createUser() so not sure why it would complain when switching over to the Astronomy code. The user variable is always being set to the current user user.

Also, this only runs once at start up after a meteor reset so the users collection is empty.

lukejagodzinski commented 9 years ago

Are you sure that when using a code with Astronomy your database is empty?

Maybe there is some error. Could you try creating a reproduction repository?

lukejagodzinski commented 9 years ago

Please create a reproduction repository as it's done for Meteor https://github.com/meteor/meteor/wiki/Contributing-to-Meteor#reporting-a-bug-in-meteor

purplecones commented 9 years ago

Sure, I'll create one and get back to you.

purplecones commented 9 years ago

@jagi ,

I created an example on meteorpad - (http://meteorpad.com/pad/9QwBZdo5yATs7dMuq/Astronomy%20User%20Test).

It does not replicate the issue exactly but I am still not able to create the users with Astronomy package with the code above. Hope this helps.

Use meteor toys package to look at the user collection. Ignore the meteor toys errors, not sure why they appear.

lukejagodzinski commented 9 years ago

There is a unique index on the username field. It's a default index created by the accounts package. When you use the Accounts.createUser() method, you don't provide the username field so it's never filled with any value and it works. However, you've defined the username field in the astronomy class and a default value for this field is null. And you can create multiple users with the null value because there is a unique index. There are two solution:

var user = new User();
user.set({
  /* ... */
});
Accounts.createUser(user.raw([
  'firstName',
  'lastName',
  'email'
]));
purplecones commented 9 years ago

Thanks @jagi. I'll probably use it only to update users, but not create.

lukejagodzinski commented 9 years ago

Ok no problem :)