[RSDK-10123] user defined metadata crud apis #477

JosephBorodach · 2025-03-04T14:18:31Z

Description

Add user-defined metadata CRUD operations

See ticket here

Todo

Do I need to manually test?

njooma · 2025-03-05T15:20:53Z

src/app/app-client.ts

+   * @param id The ID of the robot
+   * @returns The metadata associated with the robot
+   */
+  async getRobotMetadata(id: string): Promise<GetRobotMetadataResponse> {


You mix/match prepending the type of object to Id: locationId and organizationId, but just id for robot and robotPart. Let's try to be consistent? Even if the proto has different names, we can make the user-facing API consistent.

I don't mind one way or the other, maybe a slight preference to just id everywhere since the object is already specified in the method name, but I'm never against hyper-specificity if you want to do robotId

Which actually brings up another question -- do we want to call it robot or machine?

Thanks for calling this out - I agree we should be consistent, let's go with id!

My gut tells me robot because that's what we called it on the protos end of things, but what do you think?

If you prefer machine, that is fine by me!

I want to ask @JessamyT here for docs perspective: should we be using robot or machine? Most other functions are still using robot, so do we try for consistency, or do we start the migration to machine?

My uninformed inclination is to migrate towards machine since we do use that everywhere throughout the docs, even when talking about SDKs that use robot. Like we basically say robot = machine all over the place.

Sounds good - thanks for chiming in!!

(drive by comment since I'm referencing this for RSDK-10138)

I'm wondering if we actually want to use "machine" here right now. Like we pointed out, all other methods in this sdk (and the Go one) are centered around "robot", so as a new user, I'd find myself kinda confused as to why they're mixed (I have to reference our glossary from time-to-time to remind myself of the relationship between machines/robots, parts, components, etc).

IMO from a purely UX perspective, it would be clearer to continue with "robot" for consistency. We could add a docs note reminding users that robot = machine and eventually rename the methods all at once, probably with an interim deprecation. I'm still new to the ethos/process behind this, so totally okay if that's beyond our usual scope of effort!

I can see that being useful. particularly when searching for functions, you could try to do a find for robot and see all the functions available on a robot. @JessamyT can we call this robot and then to a formal migration when we're ready to rename everything to machine?

Sounds good to me!

@njooma @JessamyT - is it ok with you if we stick with robot in this pr and we cut a ticket to migrate everything over to machine at some point? I do not feel strongly one way or the other

src/app/app-client.ts

stuqdog

A small suggestion, otherwise looks good!

src/app/app-client.ts

njooma

The two comments I left are applicable to all methods added

njooma · 2025-03-10T19:25:00Z

src/app/app-client.ts

+  ): Promise<GetOrganizationMetadataResponse> {
+    return this.client.getOrganizationMetadata({ organizationId: id });


We should return a dictionary here. Since the rpc returns a Struct, we should be able to convert that Struct into a native JS obj to make it easier for users to work it

Do things look good as I have them now? I am having a really hard time getting the unit tests to pass (not too familiar with typescript)

Perhaps instead of using unknown you could use JsonValue or any (JsonValue would be more specific)

JsonValue works! Thanks!

njooma · 2025-03-10T19:25:40Z

src/app/app-client.ts

+   */
+  async updateOrganizationMetadata(
+    id: string,
+    data: Record<string, Any>


Similarly, we should take in a record of string to lowercase any (the typescript type), and then convert that into a pb.Struct so that it's easier for the user to use

edobranov

(left one comment that I want to make sure we're clear on, otherwise nothing that's blocking from me)

njooma

You might want to pull from main and regenerate the protos (using make build-buf) because it looks like your branch is still using the map<string, Any> proto definition rather than the Struct one

njooma · 2025-03-12T15:59:04Z

src/app/app-client.ts

+ * @returns A record whose keys match `data`,
+ *          with values unpacked from Struct messages
+ */
+export const decodeMetadataMap = (data: Record<string, Any>): Record<string, JsonValue> => {


Is this still needed? I don't believe we're using protobuf.Any anywhere now

Nope! thanks for clarifying

njooma · 2025-03-12T15:59:47Z

src/app/app-client.ts

+    id: string
+  ): Promise<Record<string, JsonValue>> {
+    const response = await this.client.getOrganizationMetadata({ organizationId: id });
+    return decodeMetadataMap(response.data);


You should just be able to return response.data.toJson(), no?

Yea, that basically works - needed to convert it to Record<string, JsonValue>

njooma · 2025-03-12T16:00:12Z

src/app/app-client.ts

+    const convertedData = new Struct({ fields: {} });
+    for (const [key, val] of Object.entries(data)) {
+      convertedData.fields[key] = Value.fromJson(val);
+    }


similarly, i don't think you have to convert any fields, just use Struct.fromJson(...)

yes, thanks!

edobranov

LGTM % a couple docstring tweaks

edobranov · 2025-03-12T17:54:18Z

src/app/app-client.ts

+   * Updates user-defined metadata for a machine robot.
+   *
+   * @param id The ID of the machine robot


Suggested change

* Updates user-defined metadata for a machine robot.

*

* @param id The ID of the machine robot

* Updates user-defined metadata for a robot part.

*

* @param id The ID of the robot part

edobranov · 2025-03-12T17:55:09Z

src/app/app-client.ts

+   * Retrieves user-defined metadata for an organization.
+   *
+   * @param id The ID of the organization
+   * @returns The metadata associated with the organization as a plain JS object


Suggested change

* @returns The metadata associated with the organization as a plain JS object

* @returns The metadata associated with the organization

The other docstrings don't mention this so I think we can remove it

Thanks for calling this out!

njooma

Nice!! Thanks for all the updates.

For future reference, you could also do something like this so that you aren't doing the same cast over and over again:

return (response.data?.toJson() as JsonObject) ?? {};

user defined metadata crud apis

e50f6bf

JosephBorodach requested a review from a team as a code owner March 4, 2025 14:18

JosephBorodach requested review from stuqdog, purplenicole730 and njooma and removed request for njooma March 4, 2025 14:18

njooma requested changes Mar 5, 2025

View reviewed changes

Change method signatures to use id as argument

0c1fbcd

JosephBorodach commented Mar 5, 2025

View reviewed changes

src/app/app-client.ts Outdated Show resolved Hide resolved

eslint-disable-next-line @typescript-eslint/no-explicit-any

de976ce

JosephBorodach requested a review from njooma March 5, 2025 16:31

JosephBorodach added 4 commits March 5, 2025 12:06

Run prettier with write

4c889e4

Change robot to machine

9c03916

Update any to use protobuf's any

fef2742

ran pretty write

adef311

stuqdog requested changes Mar 6, 2025

View reviewed changes

src/app/app-client.ts Outdated Show resolved Hide resolved

src/app/app-client.ts Outdated Show resolved Hide resolved

src/app/app-client.ts Outdated Show resolved Hide resolved

src/app/app-client.ts Outdated Show resolved Hide resolved

Change update metadata methods to return void

90e1a6f

JosephBorodach requested a review from stuqdog March 6, 2025 15:22

stuqdog approved these changes Mar 6, 2025

View reviewed changes

Merge branch 'main' into RSDK-10123

d51255e

njooma requested changes Mar 10, 2025

View reviewed changes

edobranov requested changes Mar 10, 2025

View reviewed changes

Update get metadata methods to return maps

72f92f0

JosephBorodach force-pushed the RSDK-10123 branch from 081d380 to 72f92f0 Compare March 11, 2025 20:35

JosephBorodach added 3 commits March 11, 2025 16:49

Modify updateOrganizationMetadata and add unit tests

fbb0976

Complete get metadata methods and add unit tests accordingly

b16e690

Modify the update methods

f4be611

njooma requested changes Mar 12, 2025

View reviewed changes

Add unit tests for update methods

fe08662

JosephBorodach added 2 commits March 12, 2025 12:37

Merge branch 'main' into RSDK-10123

6978ff0

Fix get methods

4723a36

JosephBorodach force-pushed the RSDK-10123 branch from 6a71c4b to 4723a36 Compare March 12, 2025 16:53

Add update metadata unit tests. cleanup

19f966d

JosephBorodach requested review from edobranov and njooma March 12, 2025 17:47

run formatter

6ce2a38

edobranov approved these changes Mar 12, 2025

View reviewed changes

JosephBorodach added 2 commits March 12, 2025 14:38

update comment for getOrganizationMetadata

3f85a84

Update comment for updateRobotPartMetadata

86b2331

njooma approved these changes Mar 12, 2025

View reviewed changes

JosephBorodach merged commit 35c99e0 into main Mar 12, 2025
3 checks passed

JosephBorodach deleted the RSDK-10123 branch March 12, 2025 19:30

		): Promise<GetOrganizationMetadataResponse> {
		return this.client.getOrganizationMetadata({ organizationId: id });

	* @returns The metadata associated with the organization as a plain JS object
	* @returns The metadata associated with the organization

[RSDK-10123] user defined metadata crud apis #477

[RSDK-10123] user defined metadata crud apis #477

Uh oh!

Conversation

JosephBorodach commented Mar 4, 2025

Description

Todo

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JosephBorodach Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edobranov Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stuqdog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

njooma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JosephBorodach Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edobranov left a comment

Choose a reason for hiding this comment

Uh oh!

njooma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edobranov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JosephBorodach Mar 5, 2025 •

edited

Loading

edobranov Mar 10, 2025 •

edited

Loading

JosephBorodach Mar 11, 2025 •

edited

Loading