Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

feat(config): Mongo Seed 2.0 #1808

Merged
merged 6 commits into from
Aug 13, 2017
Merged

Conversation

mleanos
Copy link
Member

@mleanos mleanos commented Jul 4, 2017

Adds a more configurable and easily extended MongoDB Seed feature.

Adds additional options at the collection, and collection item level to
allow more control over how each environment config handles the seeding
feature.

Collection level options:
skip: { when: {} }: Provides a configurable way to skip seeding a
particular collection based on results of a Mongoose query.
logResults: true/false: Overrides global Seed logResults settings.

Document level option:
overwrite: true/false: Specificy whether or not to overwrite a
document if it already exists.

Fixed tests due to SeedDB changes.

@mleanos
Copy link
Member Author

mleanos commented Jul 4, 2017

This is a modified version of changes that I've made to an existing MEANJS application. I found it rather hard to use the existing SeedDB feature. I found myself wanting more control over how the User's were added to the database, and to have the ability to add more User's.

In addition to the limitation I found, I also wanted to be able to seed other data models. This implementation might be a little wonky with the options that I've provided. However, something along these lines would be very for users of this framework. I'm open for suggestions on improvements here.

In my other project, I also created a couple Gulp tasks that I can run the SeedDB feature from. This made it much easier for me to use both in my dev & production environments. I'll add the Gulp tasks to this PR, if this gets any positive feedback.

@mleanos
Copy link
Member Author

mleanos commented Jul 6, 2017

@lirantal Any ideas on the coverage? I don't see how it could have decreased, let alone by that much. I can't make sense of the Coveralls report either.

Copy link
Member

@lirantal lirantal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generalization of it is interesting, I'm ok with that.
Looked at the coverage report as well and the changes related to the frontend routing which seems odd, as you mentioned.

@simison
Copy link
Member

simison commented Jul 11, 2017

I found it rather hard to use the existing SeedDB feature. I found myself wanting more control over how the User's were added to the database, and to have the ability to add more User's.

+1 — I never merged that feature from upstream to Trustroots cuz it seemed kinda cumbersome. Instead we've used our own which we wrote before seeding was available in mean.js. Ours is quite hacky so I would be glad to give this a test.

I'd like having this as a command npm run seed rather than as a variable/global/whatnot.

@mleanos mleanos force-pushed the feature/mongo-seed-2.0 branch from 620dc01 to 27489f5 Compare July 11, 2017 23:41
@mleanos
Copy link
Member Author

mleanos commented Jul 12, 2017

@lirantal @simison I've added Gulp tasks (& npm scripts) to run the seeding. I've left the "whatnot" implementation there, but I'm not opposed to completely removing it in this PR. Now that there are NPM scripts to run these operations, users of this framework can add them to their own builds/workflows.

I'm glad to see positive feedback here. Now moving on, there are a couple remaining issues that I could use input on..

I haven't been able reconcile the missing User reference in the Article seed. Currently, we can't guarantee when either seeding operations will complete, so we can't just look up a User. One way we could solve this limitation is by changing the SeedDB configuration object around to include the Users model in a "run this first" setting; something like:

seedDB: {
    seed: process.env.MONGO_SEED === 'true',
    options: {
      logResults: process.env.MONGO_SEED_LOG_RESULTS !== 'false'
    },
    prerequisite: {
      collections: [{
        model: 'User',
        docs: [{
          data: {
            username: 'local-admin',
            email: 'admin@localhost.com',
            firstName: 'Admin',
            lastName: 'Local',
            roles: ['admin', 'user']
          }
        }, {
          // Set to true to overwrite this document
          // when it already exists in the collection.
          // If set to false, or missing, the seed operation
          // will skip this document to avoid overwriting it.
          overwrite: true,
          data: {
            username: 'local-user',
            email: 'user@localhost.com',
            firstName: 'User',
            lastName: 'Local',
            roles: ['user']
          }
        }]
      }]
    },
    collections: [{
      model: 'Article',
      options: {
        // Override log results setting at the
        // collection level.
        logResults: false
      },
      skip: {
        // Skip collection when this query returns results.
        // e.g. {}: Only seeds collection when it is empty.
        when: {} // Mongoose qualified query
      },
      docs: [{
        data: {
          title: 'First Article',
          content: 'This is a seeded Article for the development environment'
        }
      }]
    }]
  }

Seems a little weird to me. Another way might be to check for the existence of the User model in the SeedDB collections setting, and ensure that operation completes before any other model is seeded.

Any thoughts on that? Any suggestions would be very welcome. @simison How did you handle this case over at Trustroots?

Lastly, do y'all think these are breaking changes? Any customized SeedDB settings (in the previous format) that any of our users are relying on would be invalid based on the new config structure. I just feel that not many users are using this feature as it currently stands; this is, in part, why I've introduced these changes.

@mleanos mleanos force-pushed the feature/mongo-seed-2.0 branch 4 times, most recently from 9e18d99 to fbec184 Compare July 16, 2017 06:58
@mleanos
Copy link
Member Author

mleanos commented Jul 18, 2017

@lirantal @simison I've added a solution to the above mentioned issue with the Article's user reference. I think this is a good way to handle it, but I'm open to other ideas.

Tests are passing, and if I these latest changes are approved, I'll add couple more tests.

@mleanos mleanos requested a review from lirantal July 18, 2017 00:15
@mleanos mleanos force-pushed the feature/mongo-seed-2.0 branch from fbec184 to 13b7f7e Compare July 18, 2017 01:38
@mleanos
Copy link
Member Author

mleanos commented Jul 18, 2017

BTW, I'd like to see #1818 go in before this.

@lirantal
Copy link
Member

To be honest I think the seeding is a bit over complex and very coupled to what's going on there now with User and Articles (like this return collection.model && collection.model !== 'User';)

Maybe we can solve it on the seedDB configuration object instead of actual code?

P.S. I'm curious why you removed the arrow functions?

@mleanos
Copy link
Member Author

mleanos commented Jul 20, 2017

@lirantal Do you think the seeding is complex and coupled in this PR?

Another approach I thought about was to add a "task" (function) to the config/lib/seed service for each Model that a user of MEANJS wants to seed. Then still have a static Mongoose method on each model to seed an actual document.

pseudo code:

// config/lib/seed.js

function start(config) {
  var userSeeds = [config.seedDB.options.seedUser, config.seedDB.options.seedAdmin]
    .map(function (item) {
      return seed('User', item);
    });
  var otherSeeds = [config.seedDB.options.seedOtherModel]
    .map(function (item) {
      return seed('OtherModel', item);
    });
  var articleSeeds = [config.seedDB.options.seedArticle1, config.seedDB.options.seedArticle2]
    .map(function (item) {
      return seed('Article', item);
    });
  var moreSeeds = [config.seedDB.options.yetAnotherModel]
    .map(function (item) {
      return seed('YetAnotherModel', item);
    });

  // Process Seeds in strict order
  Promise
    .all(userSeeds) // User seeds would most likely go first in most use cases
    .then(function () {
      return Promise.all(otherSeeds);
    })
    .then(function () {
      return Promise.all(articleSeeds);
    })
    .then(function () {
      return Promise.all(moreSeeds);
    })
    .then(function () {
      console.log('All Seeds Complete');
    })
    .catch(function (err) {
      console.log('Error occurred while seeding database!');
      console.log(err);
    });
}

function seed(modelName, data) { // we could add an options param here too
  var Model = mongoose.model(modelName);

  // Model.seed instance method is responsible for deciding whether to override 
  // existing documents, or any other model specific logic.
  return Model.seed(data, config.seedDB.options.logResults); // returns Promise
}

Anytime a MEANJS user wants to seed a new model, they can easily add it to the seed service, and make sure there's a seed instance method on that Mongoose model schema.

Maybe we can solve it on the seedDB configuration object instead of actual code?

To solve what? And what did you have in mind?

I removed the arrow functions based on Mikael's comments in the mongoose-upgrade PR, when he said that we should probably have a bigger discussion before we start adding ES6 features.

@lirantal
Copy link
Member

sorry if I wasn't clear before - I meant that processing the user seed first before the articles should not be really coupled to the seeding execution because then someone who doesn't need that exactly will need to edit seed.js

@mleanos
Copy link
Member Author

mleanos commented Jul 24, 2017

@lirantal I think in most cases, whatever data model's are added to a project, the User model would need to be seeded first. Anytime a model's field references a User, it would be missing if there's no user yet. The coupling of the User & Article models seems appropriate since MEANJS's Article's require a User reference.

As for a MEANJS user editing seed.js, I had that same position as you. However, this framework is meant as an example of best practices & a way to get an app up & running fast. A MEANJS user doesn't have to worry about figuring out how to do certain things that are common among most applications. This is also true of database Seeding. Once a user starts to add their own models, they would just need to modify seed.js to suit their own needs. I don't see this as much difference from the rest of the application's boilerplate code/configurations.

Maybe we can solve it on the seedDB configuration object instead of actual code?

Did you have anything specific in mind?

EDIT: Did you mean that we could enforce the ordering using the configuration object? If so, this might allow our seeding feature to remain agnostic to specific data models?? I could see a interesting implementations that could allow this to work. WDYT?

Should we just not seed Articles? If so, our user's would have little value in our Seeding feature other than to seed a couple users.

Liran, what did you think of my previous example with multiple seeds & the ordering? It seems like a pretty clean implementation. I know it's still tightly coupled, but we might not have any choice.

@lirantal
Copy link
Member

yeah @mleanos I was referring to putting that logic in the configuration object, this way user's can remove it easily without needing to alter code.
We're aligned with the framework target and audience, but just saying that not everyone's use-case includes users and logins. Some users may use it for non-user related stuff.

Let me know if you want to follow-up on the configuration object to control seed ordering.

@mleanos
Copy link
Member Author

mleanos commented Jul 26, 2017

@lirantal Should we just consider the order of the seed collections, and data, in the config object to be the order of the seed?

@lirantal
Copy link
Member

that's what I think, yes.

@mleanos
Copy link
Member Author

mleanos commented Jul 26, 2017

Sounds good.. Thanks. Let me work on those changes.

@mleanos
Copy link
Member Author

mleanos commented Jul 27, 2017

@lirantal I've updated this branch to enforce the order of the seed operations based on the configuration. Let me know what you think.

I also added two new tests to confirm the seed ordering is enforced.

@mleanos mleanos force-pushed the feature/mongo-seed-2.0 branch from 6106135 to decd36d Compare July 27, 2017 22:22
@lirantal
Copy link
Member

LGTM, thanks.

@mleanos
Copy link
Member Author

mleanos commented Jul 30, 2017

@simison Have you had a chance to test this feature out? I'm curious if it would work for you use case.

There are convenient Gulp tasks for seeding data with dev, test, and prod environment settings. You'll just have to ensure the seedDB settings have been updated in your env configs 😸

npm run seed
npm run seed:test // could be used as a "dry run" test
npm run seed:prod

@simison
Copy link
Member

simison commented Aug 1, 2017

@mleanos nope didn't have time and not sure when I'll have more time to really dig into this. Looking forward though!

mleanos added 6 commits August 3, 2017 13:50
Adds a more configurable and easily extended MongoDB Seed feature.

Adds additional options at the collection, and collection item level to
allow more control over how each environment config handles the seeding
feature.

Collection level options:
`skip: { when: {} }`: Provides a configurable way to skip seeding a
particular collection based on results of a Mongoose query.
`logResults: true/false`: Overrides global Seed logResults settings.

Document level option:
`overwrite: true/false`: Specificy whether or not to overwrite a
document if it already exists.

Fixed tests due to SeedDB changes.

chore(config): Previous SeedDB config

Removes the previous SeedDB config file.

Test coverage was decreased, and having this file here may be
contributing to the decrease.

Model options & chalk

Minor refactor to the parameters passed into the Model's seed method.

Adds chalk to messages logged to the console for readability.

feat(config): Mongo Seed config tests

Refactors the Mongo Seed configuration tests.

Adds more test coverage to the Mongo Seed feature, by adding Article
specific unit tests.

Minor sanitation checks for Mongo Seed `start` method, for options &
giving preference to configuration object passed directly in.

feat(config): More Mongo Seed Coverage

Adds additional coverage to the Mongo Seed feature.

Added logging to some of the tests to allow coverage of logs within the
Mongo Seed feature.

feat(gulp): Mongo Seed tasks

Adds Gulp tasks to perform Mongo Seed operations for default, prod, and
test environment configurations.

Also, adds accommodating npm scripts.

feat(test): Mongo Seed Configuration logging

Removes the `logResults` option from the server-side configuration
tests, in favor of using the Test environment setting (which defaults to
`false`).

This change will prevent unwanted (and unsightly) console logs during
the test coverage runs.
Seeds the User collection first (if specified in config), and then
performs remaining collection seeds.

Allows all other (non User) collection seeds to find a reference to an
Admin User, if it's necessary for the collection's model.

Adds the Admin User reference to the Article seed.
Removes the use of ES6 arrow functions.
Enforces seed order based on the order of the seeding.

Added two new tests to confirm seed ordering is enforced:

should seed custom article with user set to custom seeded admin user
should seed custom article with NO user set due to seed order
Minor modifications to how the Seed options are used. Preventing options
to override existing Seed options from configurations.

Modified the server config test clean up logic.
Minor refactoring of the configuration & options management within the
seed operations.

Minor changes to test names & variable naming.

Fixed Article document log results spacing.

Updates development env config to show Seed results by default.
@mleanos mleanos force-pushed the feature/mongo-seed-2.0 branch from 97771c5 to 66fec98 Compare August 3, 2017 20:51
@mleanos
Copy link
Member Author

mleanos commented Aug 4, 2017

This can probably go in now.. Feel free to merge. Or I can in a couple of days, if I don't hear any additional feedback.

@simison simison added this to the 0.6.0 milestone Aug 11, 2017
@simison simison mentioned this pull request Aug 11, 2017
5 tasks
@simison
Copy link
Member

simison commented Aug 13, 2017

@lirantal @mleanos good for merge?

@mleanos
Copy link
Member Author

mleanos commented Aug 13, 2017

Yes. It's ready to me.

I hesitated too see if anyone came along and also wanted to make sure this went in with a major release bump.

Good to go 😀

@simison
Copy link
Member

simison commented Aug 13, 2017

@mleanos cool, so you can squash and merge as you probably know best how the commit message should be written for this.

also wanted to make sure this went in with a major release bump.

You mean v1.0.0? As far as I know meanjs doesn't follow semver. Next up would be v0.6.0.

@mleanos
Copy link
Member Author

mleanos commented Aug 13, 2017

Meant minor bump 🤗 You're correct!

@mleanos
Copy link
Member Author

mleanos commented Aug 13, 2017

I'll merge later today. Been busy last few days. Just moved and still getting settled in. Love seeing all the contributions happening now 👍

@mleanos mleanos merged commit eb9cdd7 into meanjs:master Aug 13, 2017
cicorias pushed a commit to JavaScriptExpert/mean that referenced this pull request Sep 12, 2017
feat(config): Mongo Seed 2.0

Adds a more configurable and easily extended MongoDB Seed feature.

Adds additional options at the collection, and collection item level to
allow more control over how each environment config handles the seeding
feature.

Enforces seed order based on the order of the  environment's seeding configuration object.

Removes the previous SeedDB config file.

Adds chalk to messages logged to the console for readability.

Refactors the Mongo Seed configuration tests.

Adds Gulp tasks to perform Mongo Seed operations for default, prod, and
test environment configurations. Also, adds accommodating npm scripts.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants