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

Maps Nested unknown attributes not set when saveUnknown is true #323

Closed
fishcharlie opened this issue Mar 22, 2018 · 14 comments · Fixed by #522 or #574
Closed

Maps Nested unknown attributes not set when saveUnknown is true #323

fishcharlie opened this issue Mar 22, 2018 · 14 comments · Fixed by #522 or #574

Comments

@fishcharlie
Copy link
Member

Maps Nested unknown attributes not set when saveUnknown is true. This issue is related to #236.

@jignesh248
Copy link

@fishcharlie I wish to contribute, to fix this bug and nested partial updates bug, both are troubling me too much. I wish to fix them but I fail to understand the flow of execution of code. Is there any way that you or somebody can put a walk through of the basic code in plain english, so that it is easy to follow code.

Thanks

@fishcharlie
Copy link
Member Author

@jignesh248 Awesome!! Honestly I haven't looked through this section of the code very much. Do you have any specific questions or are you looking for a broad overview?

@jignesh248
Copy link

just looking for a broad overview.

Honestly I haven't looked through this section of the code very much.

I dunno in other thread you have created a PR for saveUknown failing silently even on top level, I guess that part is working on version released on NPM but only nested unknown field fails .

@fishcharlie
Copy link
Member Author

@jignesh248 Yeah I jut moved that to a separate issue (this one). I would look into that PR that was created for the top level stuff and see if you can replicate it for nested items. Sadly that's the only insight I have right now.

@mbejda
Copy link

mbejda commented Jan 20, 2019

This is still an issue. I don't think that PR resolved it.
Here is how the issue can be replicated :
Model and Schema

const dynamoose = require('dynamoose');
const shortId = require('shortid');
const Schema = dynamoose.Schema;
const schema = new Schema({
    adUrlId: {
        type: String,
            rangeKey: true,
    default: shortId.generate
    },
    customerId:{
        type: String,
            required: true,
            hashKey: true
    },
    userId: {
        type: String,
            required: true
    },
    title : {
        type: String,
            required: true
    },
    type: {
        type: String,
            required: true
    },
    desktop: Map
        mobile: Map,
    app: Map,
    campaignId: {
        type: String,
        required: true,
        index: {
            global: true,
            rangeKey: 'customerId'
        }
    }
});

const Model = dynamoose.model('AdUrl', schema,{
    update: true,
    timestamps: true,
    useDocumentTypes: true,
    saveUnknown: ['mobile'],
    throughput: {
        read: 1,
        write: 1
    }
});

module.exports = Model;

Saving New model with a nested mobile property "123".

    try {
        const AdUrlModel = req.app.locals.MODELS.AdUrl;
        const adUrlModel = new AdUrlModel({
            title: '123',
            type: 'cats',
            userId: 123,
            customerId: 123,
            campaignId: 123,
            mobile: {
                "123" : '123'
            }
        });
        const results= await adUrlModel.save();
        res.json(results);
    }catch(e){

    }

Outcome

{
    "title": "123",
    "type": "cats",
    "userId": "123",
    "customerId": "123",
    "campaignId": "123",
    "mobile": {},
    "adUrlId": "r3szmtJs9"
}

Mobile property did not save.....

@fishcharlie
Copy link
Member Author

@dobrynin Thoughts on this?

@fishcharlie fishcharlie reopened this Jan 20, 2019
@dobrynin
Copy link
Contributor

I think the problem may be that the changes I applied were only to parseDynamo. I think something similar needs to be done in the map section of Attribute#toDynamo

@fishcharlie
Copy link
Member Author

@dobrynin Any chance you could create a PR for that and add a test for this?

@bwaters57
Copy link

Curious: has there been any movement on this issue? My team is running across this problem as well, with a case similar to what mbejda described.

@dobrynin
Copy link
Contributor

dobrynin commented Feb 6, 2019

@bwaters57, sorry haven’t had the chance to look into this yet. Hopefully this weekend. If you have a solution and would like to submit a PR, you’re welcome to do that :)

@fishcharlie
Copy link
Member Author

I tried to take a look at this. I'm struggling to create a failing test case tho. I'm having the same problem in one of my projects. Not sure why I'm not able to create a failing test case. Or solve it.

@bwaters57
Copy link

If it helps make a failing case for testing, here is the generalized schema that we're working with:

const schema = new Dynamoose.Schema({
  id: {
    type: String,
    hashKey: true,
  },
  genericObject: Map,
}, { useDocumentTypes: true, saveUnknown: true });

And an example of this would be:

{
   'id': 'some_guid',
   'genericObject': { 
        '111_222':
         {
           id: 'some_other_guid',
           code1: 111,
           code2: 222,
         },
        '333_444':
         {
           id: 'some_other_guid',
           code1: 333,
           code2: 444,
         }
     }
}

For context, the reason the Map is generic is because the keys within are a composite of code1 and code2 that we will later be filtering on. Similar to mbejda's post above, saving and retrieving my example yields the following:

{
  id: 'some_guid',
  genericObject: {} 
}

@fishcharlie
Copy link
Member Author

@bwaters57 I think I figured this out. Working on creating a PR now.

hweeks pushed a commit to hweeks/dynamoose that referenced this issue Feb 12, 2019
this fixes how schema options can be defined and updated

fix dynamoose#323
@hweeks hweeks mentioned this issue Feb 12, 2019
8 tasks
hweeks pushed a commit to hweeks/dynamoose that referenced this issue Feb 12, 2019
this fixes how schema options can be defined and updated

fix dynamoose#323
@hweeks hweeks mentioned this issue Feb 12, 2019
8 tasks
hweeks pushed a commit to hweeks/dynamoose that referenced this issue Feb 12, 2019
this fixes how schema options can be defined and updated

fix dynamoose#323
@hweeks hweeks mentioned this issue Feb 12, 2019
8 tasks
fishcharlie pushed a commit that referenced this issue Feb 13, 2019
## [1.6.2](v1.6.1...v1.6.2) (2019-02-13)

### Bug Fixes

* **saveunknown:** fixing saveunknown toDynamo for maps ([873a6ed](873a6ed)), closes [#323](#323)
@fishcharlie
Copy link
Member Author

@mbejda One thing I just connected the dots on. Is the saveUnknown option should be part of your schema options, not model options.

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

Successfully merging a pull request may close this issue.

5 participants