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

DynRef #2640

Merged
merged 1 commit into from
Feb 3, 2015
Merged

DynRef #2640

merged 1 commit into from
Feb 3, 2015

Conversation

chetverikov
Copy link
Contributor

This pull-request provides the ability to use dynamic ref links.

Use guide

Add refPath in schema (for one field must be used or ref or refPath options):

ArrayPopulate = new Schema({
    items: [{
      // here add refPath option
      item: { type: mongoose.SchemaTypes.Mixed, refPath: 'items.type' },
      // here contains model name
      type: { type: String }
    }]
  })
mongoose.model('ap', ArrayPopulate);

refPath contains the path to the field where to store the model name.
Example data:

var ap = mongoose.model('ap');
ap.create({
  items: [
      {item: 32, type: 'DynRefReview'},
      {item: 12, type: 'DynRefPlace'},
      {item: 21, type: 'DynRefNote'}
  ]
}, function(){
  ap.find({}).populate('items.item').exec(function(err, aps){
    /**
    will contain:
    aps = {
      items:[
        { item: { here_DynRefReview_object }, type: 'DynRefReview'},
        { item: { here_DynRefPlace_object }, type: 'DynRefPlace'},
        { item: { here_DynRefNote_object }, type: 'DynRefNote'},
      ]
    }
    */
  })
})

vkarpov15 added a commit that referenced this pull request Feb 3, 2015
@vkarpov15 vkarpov15 merged commit ef42ab9 into Automattic:master Feb 3, 2015
@vkarpov15 vkarpov15 added this to the 4.0.0-rc2 milestone Feb 3, 2015
vkarpov15 added a commit that referenced this pull request Feb 6, 2015
@brandom
Copy link
Contributor

brandom commented May 15, 2015

It is very difficult to reproduce, but occasionally a dynamic ref will not populate and return null or a list of populated dynamic references will reference the wrong data entirely.

@chetverikov
Copy link
Contributor Author

@brandom Give an example of the data that you have problems. The schema and the model.

@brandom
Copy link
Contributor

brandom commented May 15, 2015

I tried to quickly clean out some of the irrelevant paths in the schema as they are rather large. Notice task_data: null in the second "occasional" response. The only thing I can think of that would cause such random behavior is a race condition in the population logic.

With larger result sets the task_data would be populated with the wrong data from a different model entirely.

Task Schema

var Task = new Schema({
    description: {
        type: String
    },
    task_type: {
        type: String
    },
    task_data: {
        type: Schema.Types.Mixed,
        refPath: 'task_type'
    },
    status: {
        type: String,
        default: 'Open',
        enum: ['Open', 'Closed']
    }
});

CSCall Schema

var CSCall = new Schema({
    caller: {
        type: String,
        required: true
    },
    notes: {
        type: String,
    }
});

AddOn Schema

var AddOn = new Schema({
    patient: {
        type: String,
        required: true
    },
    accession: {
        type: String,
        required: true
    },
    specimen: {
        type: String,
        required: true
    },
    tests: [{
        name: {
            type: String,
            required: true
        },
        code: {
            type: String,
            required: true
        }
    }]
});

Typical response

[
  {
    "_id": "55556668b40bbdc460c0a34c",
    "deleted": false,
    "updatedAt": "2015-05-15T03:22:16.998Z",
    "due": "2015-05-15T06:21:51.404Z",
    "task_data": {
      "_id": "55556668b40bbdc460c0a34d",
      "deleted": false,
      "_createdBy": "53e53f89191bb8ea1afc7e02",
      "_updatedBy": "53e53f89191bb8ea1afc7e02",
      "updatedAt": "2015-05-15T03:22:17.000Z",
      "createdAt": "2015-05-15T03:21:00.000Z",
      "caller": "Test",
      "category": "Cancellation",
      "notes": "Test cancel",
      "__v": 0,
      "id": "55556668b40bbdc460c0a34d"
    },
    "task_type": "CSCall",
    "__v": 0,
    "status": "Open",
    "id": "55556668b40bbdc460c0a34c"
  },
  {
    "_id": "555566d1af428dd762989454",
    "deleted": false,
    "task_data": {
      "_id": "555566d1af428dd762989455",
      "deleted": false,
      "_createdBy": "53e53f89191bb8ea1afc7e02",
      "_updatedBy": "53e53f89191bb8ea1afc7e02",
      "updatedAt": "2015-05-15T03:24:01.884Z",
      "createdAt": "2015-05-15T03:23:00.000Z",
      "patient": "Patient Name",
      "accession": "X1234",
      "specimen": "Blood",
      "tests": [
        {
          "name": "Vitamin  B12 and Folate Panel",
          "code": "B12FO",
          "_id": "555566d1af428dd762989456",
          "id": "555566d1af428dd762989456"
        }
      ],
      "__v": 0,
      "id": "555566d1af428dd762989455"
    },
    "task_type": "AddOn",
    "description": "Comments here.",
    "__v": 0,
    "status": "Open",
    "id": "555566d1af428dd762989454"
  }
]

Occasional response

[
  {
    "_id": "55556668b40bbdc460c0a34c",
    "deleted": false,
    "task_data": null,
    "task_type": "CSCall",
    "__v": 0,
    "status": "Open",
    "id": "55556668b40bbdc460c0a34c"
  },
  {
    "_id": "555566d1af428dd762989454",
    "deleted": false,
    "task_data": {
      "_id": "555566d1af428dd762989455",
      "deleted": false,
      "_createdBy": "53e53f89191bb8ea1afc7e02",
      "_updatedBy": "53e53f89191bb8ea1afc7e02",
      "updatedAt": "2015-05-15T03:24:01.884Z",
      "createdAt": "2015-05-15T03:23:00.000Z",
      "patient": "Patient Name",
      "accession": "X1234",
      "specimen": "Blood",
      "tests": [
        {
          "name": "Vitamin  B12 and Folate Panel",
          "code": "B12FO",
          "_id": "555566d1af428dd762989456",
          "id": "555566d1af428dd762989456"
        }
      ],
      "__v": 0,
      "id": "555566d1af428dd762989455"
    },
    "task_type": "AddOn",
    "description": "Comments here.",
    "__v": 0,
    "status": "Open",
    "id": "555566d1af428dd762989454"
  }
]

@chetverikov
Copy link
Contributor Author

@brandom Show me your query. Why in the scheme task_data: Schema.Types.Mixed?

@brandom
Copy link
Contributor

brandom commented May 15, 2015

Query for testing is simply:

Task.find({}).populate('task_data').exec(function(err, records) {
  if (err) res.send(err);
  res.send(records);
});

I used Mixed because an ObjectID is stored in MongoDB but an object is returned when populated.

Like I said it is difficult to reproduce because at the moment I am simply refreshing the route that calls the query and I get task_data: null ~ 1 in 30 queries.

@chetverikov
Copy link
Contributor Author

@brandom
Even if you specify the ObjectId, it would still have to fill the object. Or I don't know what you're +)
But, I did find a bug that you're talking about. The problem lies in the "populate" and not in DynRef. I I'll try to figure something out with this...

@brandom
Copy link
Contributor

brandom commented May 15, 2015

Thank you!! I just figured it was DynRef because basic population has been around for quite a while and battle tested. I hope to be using DynRef heavily in a production app in a few months so I appreciate your quick response! This was a killer PR.

@chetverikov
Copy link
Contributor Author

@brandom
Just this error very rarely manifests itself in the work. So many people just do not notice.
However, it is worth noting that "populate" it's a very bad pattern for lists. Populate should only be used when fetching a single object.
I myself have often used the populate and wrote DynRef, but later realized that it was very bad. And rebuilt my app. Now it works faster and more efficiently than before.

@brandom
Copy link
Contributor

brandom commented May 15, 2015

Thanks for the tip. At the moment I am limiting task queries to the top 10 so performance is acceptable. I may refactor closer to production. Please let me know when you have a fix for this as I may run off of a fork until it gets merged.

@chetverikov
Copy link
Contributor Author

Ok

@vkarpov15
Copy link
Collaborator

Can you guys open up a separate issue for this? Easier to track.

Also @chetverikov is right, populate on lists is often a bad design choice. It's great for the occasional case when you have a many-to-many relationship where the "many" is small on one side, but people often end up using it for the "arrays which grow without bound" anti-pattern.

@brandom
Copy link
Contributor

brandom commented May 15, 2015

I'll open one as soon as I get to my computer.

In my use case I am populating a short list with a 1:1 relationship where one side is dynamic.

Sent from my iPhone

On May 15, 2015, at 9:41 AM, Valeri Karpov notifications@github.com wrote:

Can you guys open up a separate issue for this? Easier to track.

Also @chetverikov is right, populate on lists is often a bad design choice. It's great for the occasional case when you have a many-to-many relationship where the "many" is small on one side, but people often end up using it for the "arrays which grow without bound" anti-pattern.


Reply to this email directly or view it on GitHub.

@brandom
Copy link
Contributor

brandom commented May 16, 2015

Sorry for the delay, see #2992 with code to reproduce.

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

Successfully merging this pull request may close these issues.

3 participants