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

Dust doesn't render a result set obtained from MongoDB (by Mongoose) #487

Closed
wzup opened this issue Jul 2, 2014 · 24 comments
Closed

Dust doesn't render a result set obtained from MongoDB (by Mongoose) #487

wzup opened this issue Jul 2, 2014 · 24 comments

Comments

@wzup
Copy link

wzup commented Jul 2, 2014

I have an issue: Dust doesn't render a result set from MongoDB by Mongoose. But a custom made array of objects renders just fine. What is wrong? Why it happens? How to manage it? I use Windows 7 64bit if it matters.

Here is a code.

This is my controller. Pay attention to two model objects. The first one (custom made) is
rendered fine. The second one (obtained from MongoDB) is not rendered.

    router.get('/', function (req, res) {

        Post.find(function (err, posts) {
            if (err) {
                console.log(err);
            }

            //
            // This is rendered just fine
            //
            var model = {
                posts: [
                    {title: "Title1", body: "Body1"},
                    {title: "Title2", body: "Body2"}
                ]
            };

            //
            // This model is not rendered by Dust
            //
            var model = {
                posts: posts
            }

            res.render('posts', model);
        });
    });

A result set from MongoDB that is not rendered. Its Schema:

    var postSchema = mongoose.Schema({
        title: String,
        body: String,
        crAt: { type: Date, default: Date.now },
        upAt: { type: Date, default: Date.now }
    });

My .dust template code:

{>"layouts/master" /}
{<title}
    Posts
{/title}

{<body}
    {>"partials/header" /}
    <div id="container" class="container">

    <ul>
    {#posts}
        <li>
            {.title}
            <br>
            {.body}
        </li>
    {/posts}
    </ul>

        {>"partials/footer" /}
    </div>
{/body}
@wzup wzup changed the title Dust doesn't render a result set obtained from MongoDB Dust doesn't render a result set obtained from MongoDB (Mongoose) Jul 2, 2014
@wzup wzup changed the title Dust doesn't render a result set obtained from MongoDB (Mongoose) Dust doesn't render a result set obtained from MongoDB (by Mongoose) Jul 2, 2014
@wzup
Copy link
Author

wzup commented Jul 2, 2014

Here is the same issue by the way: #479

I also confirm that JSON.parse(JSON.stringify(entries)) works for me too - the result set obtained from Mongoose is rendered.

But why it is not rendered without JSON.parse, JSON.strigify?

@jimmyhchan
Copy link
Contributor

Is this mongo specific? Can you clarify what the dust issue is?

@wzup
Copy link
Author

wzup commented Jul 2, 2014

@jimmyhchan it doesn't render the data. The data comes from MongoDB, I can see the data in console.log(...), but Dust for some reason doesn't render it.

@jimmyhchan
Copy link
Contributor

Is it only a problem in 2.4. If so, one thing that did change was we stopped looking in the object prototype when determining the value of a reference.

I'm guessing but does Mongo(oose) return a simple object or something wrapped up. That is, does the mongoose object have more methods then a simple Object/array. Another thing to check is the prototype and constructor of the object and compare that against a simple var o = {}.

Your entries object may be more complicated and only looks like a simple object. This may explain why console.log and parse(stringify(entries)) appear to work.

@thomasreiser
Copy link

I have the same problem using a Mongoose object and Dust 2.4.0 (KrakenJS).
Are there any workarounds or fixes known?

@jimmyhchan
Copy link
Contributor

It appears Mongoose returns a document (which looks like a plain JS object but is not). With Dust 2.4, we expect a real JS object. This might be what you need http://mongoosejs.com/docs/api.html#query_Query-lean.

@sixlettervariables
Copy link

Likewise this appears to not work with Sequelize objects either. My templates look pretty bad as I have to access private members, like {user.dataValues.name|h}, where res.locals.user = req.user.

Coming from Jade, it is a bit awkward to have to jump through these hoops. I'm now adding a viewmodels directory to handle marshaling back and forth between Dust's requirement for POD.

@fredoche
Copy link

@jimmyhchan Mongoose objects are real JS objects. It is really bothersome to create dedicated "flat" data structures to please Dust inner works. The use of the prototypal chain to implement proxies or inheritance in nodeJS is really widespread, and do not make JS objects 'unreal' for that matter.
Having to call query.lean() or mymodel.toObject() when I use mongoose creates coupling in my generic business code to mongoose interface as well as dustjs implementation.

@jimmyhchan
Copy link
Contributor

I don't disagree. The mongoose object and the other prototype example is very useful. Dust 2.4 not support this is still a valid bug...but at least there is a work around. The original issue #469 is/was the bigger bug (IMO).

I'm definitely open to providing support for this but need an implementation that also works for the general case.

@qraynaud
Copy link

@jimmyhchan: I don't see how #469 relates in any way to the general case. It's not up to dusjts to handle badly chosen property names overriding method names in the prototype...

I believe a helper to check ownProperty in dust helpers might fix the issue for the #469 use case. Why not revert what was done in #469 and go for a small helper for this very specific situation?

Preventing prototypes from being read is actually preventing good JS to work. This looks bad to me.

@fredoche
Copy link

I believe that the general case should be to go through the prototypal chain, as dust always did, and let users be bitten if they name their "eventually empty properties" the same as "eventually existing methods" .
A caveat in the documentation would, imho suffice to make users vigilant about it. Breaking dust for about everybody that uses inheritance and proxies is a bit over the top.

@jimmyhchan
Copy link
Contributor

So some options:

  1. revert the change for reference get should not look at the prototype #469 and document the "eventually empty properties" issue. This fixes this issue at the expense of 469 - which should be somewhat rare.
  2. keep what we have post 2.4 as the default but allow a flag or a helper to let Dust look in the prototype. (flags meh) Basically something to allows devs to opt-in to the pre 2.4 approach.

Any other options?

I'm not oppose to going back to the pre 2.4 approach, my only argument is that it's likely easier and less surprising for devs to have to opt-in to prototypal chaining (it's more obviously borken) then for devs to have to opt-in to hasownproperty checks (more rare but subtlety borken). That said, supporting both modes feels wrong.

@sixlettervariables
Copy link

Option 1 seems to be the most valid option from an end user's standpoint.

It is unexpected that a valid Javascript object fails to work when assigned to a context. It took me quite a while to figure out why my conversion from Jade to Dust was failing so miserably (there is now an entire area of my project dedicated to simply marshaling ORM data for Dust templates). It wasn't until I stumbled across this issue that I even realized my Javascript object wasn't a Javascript object ("why does it say Paper Jam when there is no paper jam!?").

@jimmyhchan
Copy link
Contributor

Chatting with a few others, seems the majority favor fixing this bug (allow prototype lookup) over the original bug. I'll try to get a pull request up to revert the previous change by next week for the next 2.4.X push.

A quick pull request for anyone who has time.

Thanks everyone for the lively discussion!

@fredoche
Copy link

Thank you very much for your feedback and action.

On Wed, Aug 27, 2014 at 5:52 PM, jimmyhchan notifications@github.com
wrote:

Chatting with a few others, seems the majority favor fixing this bug
(allow prototype lookup) over the original bug. I'll try to get a pull
request up to revert the previous change by next week for the next 2.4.X
push.

A quick pull request for anyone who has time.

Thanks everyone for the lively discussion!


Reply to this email directly or view it on GitHub
#487 (comment).

@sethkinast
Copy link
Contributor

This has been reverted in 2.4.1.

@prashn64 prashn64 closed this as completed Sep 3, 2014
@vanmartin
Copy link

I'm unsure whether to create a new issue or simply comment on this one. Seeing as it contains some useful history on the issue, I'll post my comment here.

I've just upgraded to version 2.7.1 and I've noticed that a Passportjs object retrieved from MongoDB causes my page to hang (no issues before the upgrade though). Wrapping it with the JSON.parse & JSON.stringify functions as follows does however seems to sort out the issue:

        res.render('casestudies', {
            user : JSON.parse(JSON.stringify(req.user)), 
            assessments : req.assessments
        });

What I find really strange is the same is not necessary for req.assessments with also happens to be retrieved from MongoDB.*

*EDIT: This is incorrect. Seems I run into a similar problem with req.assessments too.

This is what my User model looks like:

var userSchema = mongoose.Schema({
    local            : {
        name         : String,
        email        : String,
        password     : String,
    },
    userRoles : [Number]
});

My apologies in advance if I'm doing something daft without realising it.

@sethkinast
Copy link
Contributor

Does either dust.isThenable or dust.isStreamable return true if you pass it the req.user object? Sounds like duck typing that I can fix.

@sethkinast
Copy link
Contributor

If so, can you share the versions of mongoose and mongodb you're using?

@sethkinast
Copy link
Contributor

I tried briefly to repro but I'm sure I'm only scratching the surface. It does seem that Dust is rendering the object returned by a find, at least.

var mongoose = require('mongoose');
var dust = require('dustjs-linkedin');

mongoose.connect('mongodb://localhost/test');

var Cat = mongoose.model('Cat', { name: String });

var kitty = new Cat({ name: 'Zildjian' });
kitty.save(function (err) {
  console.log('meow');

  Cat.find(function(err, data) {
    console.log(data);

    dust.renderSource("Kittens: {#kittens}{name}{~n}{/kittens}", {
      kittens: data
    }, function(err, out) {
      console.log(out);
    });
  });
});

After running a couple times:

$ node app.js
meow
[ { _id: 55538a05ccd52aa66dbbd79d, name: 'Zildjian', __v: 0 },
  { _id: 55538a105d5791a76d45461b, name: 'Zildjian', __v: 0 },
  { _id: 55538a2010a650aa6d408075, name: 'Zildjian', __v: 0 } ]
Kittens: Zildjian
Zildjian
Zildjian

@sethkinast
Copy link
Contributor

It looks like mongo Documents are Streamables, so my guess is that they aren't being ended or are ALREADY ended. Once you come back with more info we'll be able to narrow this down.

@vanmartin
Copy link

Does either dust.isThenable or dust.isStreamable return true if you pass it the req.user object? Sounds like duck typing that I can fix.

dust.isThenable returns false
dust.isStreamable returns true

If so, can you share the versions of mongoose and mongodb you're using?

Mongoose: 3.8.28
MongoDB: 2.6.7

I tried briefly to repro but I'm sure I'm only scratching the surface.

I can confirm that we managed to reproduce the issue on another dev's machine as well.

Please let me know if you need more info. Happy to help get this resolved although I can confirm that everything plays nicely when reverting back to dust 2.6.2

@sethkinast
Copy link
Contributor

Thanks, I was able to reproduce it.

So Documents in Mongo can be asynchronously delivered, but I don't think
they conform to the normal Streams API. I'm going to dig into how to best
support them in Dust, whether that means submitting a patch to upstream or
being pickier about what Dust thinks a stream is.

If you want to work around it for now, address Mongo Documents as
references, not as sections.

So instead of:

{#req.user}{name}{/req.user}

Use:

{req.user.name}

Dust 2.7.0 added Streamable support which is why you don't see it before
then.

Thanks for the report! We'll get this behavior improved as soon as we can.

On Thu, May 14, 2015 at 5:02 AM, vanmartin notifications@github.com wrote:

Does either dust.isThenable or dust.isStreamable return true if you pass
it the req.user object? Sounds like duck typing that I can fix.

dust.isThenable return false
dust.isStreamable return true

If so, can you share the versions of mongoose and mongodb you're using?

Mongoose: 3.8.28
MongoDB: 2.6.7

I tried briefly to repro but I'm sure I'm only scratching the surface.

I can confirm that we managed to reproduce the error on another dev's
machine as well.

Please let me know if you need more info. Happy to help get this resolved
although I can confirm that everything plays nicely when reverting back to
dust 2.6.2


Reply to this email directly or view it on GitHub
#487 (comment).

Seth Kinast
http://sethkinast.com/

@vanmartin
Copy link

Thanks! I managed to work out that it breaks on sections but not references by a process of elimination earlier in the week but honestly there's no immediate pressing need for me to upgrade my Dust version. Reverting back to 2.6.2 is my best option at the moment as I use sections in quite a number of my existing templates.

I'll keep following the conversation on issue #663.

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

No branches or pull requests

9 participants