dynamoose icon indicating copy to clipboard operation
dynamoose copied to clipboard

question: `update` an item map attributes instead of replacing the map

Open diosney opened this issue 5 years ago • 14 comments

Summary:

I'm trying to implement a serverless framework/blueprint to integrate ORM, a RESTFul HTTP interface, enforce a lot of opinionated constrains to minimize discussion between team.

One of those constrains needed to integrate JSON-API (https://jsonapi.org) PATCH queries is that the updates have to be atomic, so if I send a patch with address: { postalCode: 1231 }, being address a map, it must not replace the entire map, but only update the nested postalCode.

The issue is that however I use .update, it overrides the entire map instead of replacing only the address.postalCode value.

Sorry if below I input only the object definitions instead of the full calls, it is since those are generated dynamically from those objects.

Code sample:

Schema

{
   id       : {
      type    : String,
      hashKey : true,
      readonly: true,
      default : () => {
        return uuid.v4();
      }
    },
    address  : {
      type: 'map',
      map : {
        streetAddress      : {
          type    : String,
          required: true
        },
        state              : {
          type    : String,
          required: true
        },
        country            : {
          type    : String,
          required: true
        },
        postalCode         : {
          type    : Number,
          required: true
        },
        postOfficeBoxNumber: {
          type: Number
        }
      }
    }
}

Model

It is dynamically generated from the above schema object. It is named "user".

General

I've tried a lot of update calls:

this.update(key, {
   address: {
       postalCode: 2312
   }
});

this.update(key, {
    $PUT: {
        address: {
          postalCode: 2312
       }
   }
});

this.update(key, {
    $ADD: {
        address: {
          postalCode: 2312
       }
   }
});

but none of them works, $PUT replaces the entire map whereas $ADD throws an error saying that a MAP it is not a valid operator for $ADD.

Current output and behavior:

However I use .update, it overrides the entire address map instead of replacing only the address.postalCode value.

Expected output and behavior:

A way to only replace the address.postalCode nested map item.

Environment:

Operating System: Kubuntu Operating System Version: 18.04 Node.js version (node -v): v12.8.1 NPM version: (npm -v): 6.10.2 Dynamoose version: 1.10.0 Dynamoose Plugins: None

Dynamoose Plugins:

  • [ ] Yes, I believe that one or multiple 3rd party Dynamoose plugins are effecting this issue
  • [ ] No, I believe this issue is independent of any 3rd party Dynamoose plugins I'm using
  • [ ] Unknown, I'm unsure if Dynamoose plugins are effecting this issue
  • [X] I am not using any Dynamoose plugins

Other information (if applicable):

Type (select 1):

  • [ ] Bug report
  • [ ] Feature suggestion
  • [X] Question
  • [ ] Other suggestion
  • [ ] Something not listed here

Other:

  • [X] I have read through the Dynamoose documentation before posting this issue
  • [X] I have searched through the GitHub issues (including closed issues) and pull requests to ensure this issue has not already been raised before
  • [X] I have searched the internet and Stack Overflow to ensure this issue hasn't been raised or answered before
  • [ ] I have tested the code provided and am confident it doesn't work as intended
  • [X] I have ensured that all of my plugins that I'm using with Dynamoose are listed above
  • [X] I have filled out all fields above
  • [X] I am running the latest version of Dynamoose

diosney avatar Aug 22 '19 19:08 diosney

To momentarily solve this I'm doing:

  • a pre-Get on the item if the value is a map,
  • detecting if the schema has a map attribute,
  • patching the whole map with the new values
  • passing the new map to the initial whole item values
  • updating

but this needs an extra Get requests that I want to eliminate, so it could be nice if a one-hit update query could be done.

Thanks

diosney avatar Aug 22 '19 21:08 diosney

This is a limitation with Dynamoose currently. At some point I hope to fix this, but for now the only way is to call the AWS-SDK directly.

Definitely gotta figure out the syntax will work for users to call this. But it’s something that I did become more aware of for version 2.0.

fishcharlie avatar Jan 24 '20 05:01 fishcharlie

Looks like this is related to https://github.com/dynamoosejs/dynamoose/issues/398 and https://github.com/dynamoosejs/dynamoose/pull/644. The best way of doing it is to use the SET update expression (I've described it here - https://github.com/dynamoosejs/dynamoose/pull/644#issuecomment-506465236), which seems to not have been fully implemented by Dynamoose yet, but you should be able to fetch and update just the single attribute, as opposed to the whole document. Also note that in case you want to be able to update arbitrary nested values, prefetching and updating the entire attribute might be a better option anyway.

dolsem avatar Feb 10 '20 20:02 dolsem

This issue is stale because it has been open 7 days with no activity. Remove stale label or comment or this will be closed in 3 days.

github-actions[bot] avatar May 11 '20 00:05 github-actions[bot]

This is a feature I've needed a few times now, and I'm totally willing to tackle it if we can come up with a sane implementation. Mongoose uses the object.prop syntax to update the prop property on an object called object. We could theoretically do the same, but it would break updates for attributes with dots . in the name. I don't think there are any restrictions on DynamoDB attribute names, so any character-delimited update like Mongoose does could potentially break existing updates.

That said, we could make the feature available only when explicitly enabled in the configuration, or introduce a new $SET_MAP or something along those lines. Open to ideas!

andrewda avatar Mar 04 '21 10:03 andrewda

@andrewda

This is a feature I've needed a few times now, and I'm totally willing to tackle it if we can come up with a sane implementation

That would be amazing!! Mind writing up a proposal for how you think this should work in terms of syntax and changes we'll make to implement this? Then after I approve that you can start on a PR maybe??

but it would break updates for attributes with dots . in the name

Technically Dynamoose itself adds a restriction to attribute names with dots . in the name. You can view the implementation of this below.

Although it might be allowed in DynamoDB, it creates too many complexities in Dynamoose due to how many times we split on . and use it to determine nested attributes.

https://github.com/dynamoose/dynamoose/blob/8a1a1d94382793d070da9800295117c247ad32e7/lib/Schema.ts#L548-L560

fishcharlie avatar Mar 04 '21 15:03 fishcharlie

That would be amazing!! Mind writing up a proposal for how you think this should work in terms of syntax and changes we'll make to implement this? Then after I approve that you can start on a PR maybe??

For sure, I'll write that up in a bit.

Technically Dynamoose itself adds a restriction to attribute names with dots . in the name. You can view the implementation of this below.

Although it might be allowed in DynamoDB, it creates too many complexities in Dynamoose due to how many times we split on . and use it to determine nested attributes.

Ah good to know! That will probably make things easier!

andrewda avatar Mar 04 '21 21:03 andrewda

@fishcharlie

So I'm thinking we can implement something along the lines of this:

const innerSchema = new Schema({
  prop1: String,
  prop2: Number,
  prop3: { type: Array, Schema: [Number] },
})

const schema = new Schema({
  id: { type: String, hashKey: true },
  inner: innerSchema,
})

// ...

model.update({ id: '123' }, {
  $SET: {
    'inner.prop1': 'hello world',
    'inner.prop2': 42,
  },
})

// {
//   TableName: 'model',
//   ExpressionAttributeNames: { '#a0': 'inner', '#a1': 'inner', '#p0': 'prop1', '#p1': 'prop2' },
//   ExpressionAttributeValues: { ':v0': { S: 'hello world' }, ':v1': { N: '42' } },
//   UpdateExpression: 'SET #a0.#p0 = :v0, #a1.#p1 = :v1',
//   Key: {
//     'id': { 'N': '123' },
//   },
// }

We'd likely want to do the same with $DELETE, too, for example:

model.update({ id: '123' }, {
  $REMOVE: ['inner.prop1', 'inner.prop2'],
})

// {
//   TableName: 'model',
//   ExpressionAttributeNames: { '#a0': 'inner', '#a1': 'inner', '#p0': 'prop1', '#p1': 'prop2' },
//   UpdateExpression: 'REMOVE #a0.#p0, #a1.#p1',
//   Key: {
//     'id': { 'N': '123' },
//   },
// }

DynamoDB also supports updating/deleting array properties by their index. We can support this in a similar way:

model.update({ id: '123' }, {
  $SET: { 'inner.prop3[0]': 42 },
})

// {
//   TableName: 'model',
//   ExpressionAttributeNames: { '#a0': 'inner',  '#p0': 'prop3' },
//   ExpressionAttributeValues: { ':v0': { N: '42' } },
//   UpdateExpression: 'SET #a0.#p0[0] = :v0',
//   Key: {
//     'id': { 'N': '123' },
//   },
// }

model.update({ id: '123' }, {
  $REMOVE: ['inner.prop3[0]'],
})

// {
//   TableName: 'model',
//   ExpressionAttributeNames: { '#a0': 'inner', '#p0': 'prop3' },
//   UpdateExpression: 'REMOVE #a0.#p0[0]',
//   Key: {
//     'id': { 'N': '123' },
//   },
// }

Open to any improvements or changes!

andrewda avatar Mar 05 '21 07:03 andrewda

@andrewda Couple of things:

  1. It's important to note that not all nested objects are schemas. I don't think this will impact implementation based on how it's setup. But is important to be aware of.
  2. Also important to note that you can set the update object to be {"inner.prop1": "hello world"} and that will do the same as $SET.
  3. I don't think $DELETE is allowed in this case? Need to be sure we handle that edge case.
  4. Finally. Concerned about that last example. Throughout Dynamoose currently we signify . as a nested property. Using [0] to signify that would be a change in the pattern of how we are currently doing things. I'm hesitant about diverging too much between internal patterns and external user facing API. I also forget, are brackets allowed as property names?? We don't currently have a use case to prevent that, but DynamoDB itself might not allow them. If they do allow it, that might cause conflict with property names. I think my proposal here is to change it to be $REMOVE: ['inner.prop3.0']. What do you think?

Really want to have good tests around this. Ideally we'd cover all of the following:

  • Nested Schemas
  • Nested Objects (without separate schema)
  • Nested Schemas within Arrays
  • Nested Objects within Arrays
  • $DELETE edge cases
  • With $SET and without

It'd be great if we can figure out a good way to ensure that stays DRY. Our tests right now aren't very DRY. At some point I wanna go back through and improve that. A lot of that can be done by nested it statements within forEach loops and such to spawn multiple the same test with slightly different variables.

That is all I can think about now. I like the proposal, and minus the things I said above, I think if you want, you can for sure take a shot at a PR for this. Thank you so much for your help here!!!

fishcharlie avatar Mar 05 '21 17:03 fishcharlie

Any update on this issue?

hoangnguyenba avatar Jan 21 '22 05:01 hoangnguyenba

@fishcharlie I got an idea just pass with this way to update as example below,

We still passing normal json to update but it will auto detected and just update an item map attribute. For Array will still replacing it all (I know it can support to change array item based on index but it just not reasonable for most situation).

I think it is really nice to have this in v2 & v3, since v3 also using same base code from v2 as well.

const CatSchema = new dynamoose.Schema(
	{
		id: { type: Number, hashKey: true },
		upperMidline: {
			type: Object,
			schema: {
				type: {
					type: String,
				},
				mm: {
					type: Number,
				},
				test: {
					type: Array,
					schema: [
						{
							type: Object,
							schema: {
								abcd: String,
							},
						},
					],
				},
			},
			required: true,
		},
	},
	{ timestamps: true },
);

const Cat = dynamoose.model('Cat', CatSchema);

const test = await Cat.update(666, { upperMidline: { mm: 2343243432, test: [{ abcd: '1234124125' }] } });

In update function - getUpdateExpressionObject:

const operator = updateType.operator || (updateType.attributeOnly ? "" : " ");
if (subValue instanceof Object)  {
    const updateNestedAttributes = (sv) => {
        const subValueKeys = Object.keys(sv);
        for (let i = 0; i < subValueKeys.length; i++) {
            const updateIndex = index + i;
            const subExpressionKey = `#a${updateIndex+1}`;
            expressionValue = `:v${updateIndex}`;
            accumulator.ExpressionAttributeNames[subExpressionKey] = subValueKeys[i];
            if (!Array.isArray(sv[subValueKeys[i]]) && sv[subValueKeys[i]] instanceof Object) {
                updateNestedAttributes(sv[subValueKeys[i]]);
            } else {
                subValue = sv[subValueKeys[i]];
                accumulator.ExpressionAttributeValues[expressionValue] = subValue;
                accumulator.UpdateExpression[updateType.name.slice(1)].push(`${expressionKey}.${subExpressionKey}${operator}${expressionValue}`);
            }
        }
    }
    updateNestedAttributes(subValue);
} else {
    accumulator.UpdateExpression[updateType.name.slice(1)].push(`${expressionKey}${operator}${expressionValue}`);
}
index++;

KennethWKZ avatar Jun 27 '22 22:06 KennethWKZ

Related: https://github.com/dynamoose/dynamoose/discussions/1477

fishcharlie avatar Aug 24 '22 04:08 fishcharlie

Would love to see nested properties update available in dynamoose. This is a really sweet lib to build simple model/controller architecture very fast but not being able to update only a nested property is pretty non convenient :confused:

4. Finally. Concerned about that last example. Throughout Dynamoose currently we signify . as a nested property. Using [0] to signify that would be a change in the pattern of how we are currently doing things. I'm hesitant about diverging too much between internal patterns and external user facing API. I also forget, are brackets allowed as property names?? We don't currently have a use case to prevent that, but DynamoDB itself might not allow them. If they do allow it, that might cause conflict with property names. I think my proposal here is to change it to be $REMOVE: ['inner.prop3.0']. What do you think?

To give a hint on this . is reserved for map and for list you have to use [0] (doc link) A nested UpdateExpression would then be like this SET #a1.#a2[0].#a3 = :p1 and this also work for REMOVE operation. I think the proposed format prop0.prop1.0.prop2 notation can be good one and then replace on the fly the .0 by [0] the only issue could be if this is actually a map and not a list {"foo": "bar", "0": "thing"} but would be a weird schema :thinking:

In addition, I think this is related to this one too https://github.com/dynamoose/dynamoose/issues/994 where we cannot update a nested tree.

Would really love to see this possible rather than building native update request on our own :slightly_smiling_face:

Hideman85 avatar Sep 09 '22 16:09 Hideman85

https://github.com/dynamoose/dynamoose/discussions/1526 is a similar comment for adding support for nested attributes in conditionals.

fishcharlie avatar Oct 07 '22 14:10 fishcharlie

@andrewda Did you end up doing any work on this?

tbenyon avatar Oct 15 '22 08:10 tbenyon

Any progress on this? Today again I need to swith writing a pure dynamodb update call :confused: Please support of { '$SET': { [`inviteInfo.${userID}.lastSent`]: new Date() } } :pray:

Hideman85 avatar Nov 03 '22 11:11 Hideman85

i have the same need of updating an attribute of a nested object

ThomasEcherer avatar Feb 24 '23 09:02 ThomasEcherer

Closing as a duplicate of https://github.com/dynamoose/dynamoose/issues/1589 to keep everything in one place.

fishcharlie avatar Apr 16 '23 05:04 fishcharlie