Send the whole user preferences object to the backend when saving

[ryanclark]

This changes the UI to always send the whole user preferences object when the user changes a preference. This is so we can support bool and int values in the future.

[ibeckermayer]

Confused as to exactly why this is necessary. IIUC we're planning to add boolean fields to this UserPreferences object: why can we not make these fields optional/nullable and use that to represent whether or not we wish to update them? (I'm presuming you've thought of that and have a good answer, just asking for my own understanding).

[codingllama]

Drive-by per Ryan's request.

[codingllama]

Suggested change

	// fieldmask is the field mask of the user preferences to update.
	google.protobuf.FieldMask fieldmask = 3;
	// User preferences update mask.
	// Field names are specified using the proto names, for example $EXAMPLES_HERE.
	// If absent a full update is performed, in accordance to legacy semantics.
	google.protobuf.FieldMask update_mask = 3;

Are full updates of entire fields allowed, like "assist", or does one need to specify particular fields, like "assist.preferred_logins"?

Is the comment above correct in regards to full updates/legacy?

Reference for the update_mask name: https://cloud.google.com/apis/design/standard_methods#update

[ryanclark]

At the moment it's the full path, i.e. assist.preferred_logins. This is because the frontend code doesn't check if you're updating all of the object's values to simplify assist.view_mode and assist.preferred_logins into assist.

Do you think I should add support for specifying assist?

Also, I updated all the names to update mask.

[codingllama]

There are no hard-and-fast rules for this, I think. Just document your decision and it's alright.

[codingllama]

This defeats the purpose of the field mask, I think. You are supposed to write explicitly what fields you want to touch in the updates.

[strideynet]

Agreed - field masks work best when the client sets them. Inferring field masks just moves the issues around presence detection to where you infer the field mask.

[tigrato]

To leave my two cents here:
Field masks make sense when they represent what we want to update. In this case, as we use an intermediate structure to make the JSON marshal/unmarshal, there is no 1:1 guarantee between that structure and the protobuf representation.
This means that either we do a fieldmask translation between what users send via JSON and the fieldmask values in protobuf (these are the fields name and not the json name) or else the backend has to use the representations in JSON which it does little sense since it prevents any use of the API just for protobuf.

If everyone wants to follow this path I won't block it but for me, it doesn't make much sense to ask for fieldmasks from the client without there being a 1:1 guarantee between the object they send and the protobuf object.

[codingllama]

To leave my two cents here (...)

Agree with @tigrato - the mask paths are supposed to come from the originator of the update, otherwise we are mostly shifting the problem around.

[codingllama]

There are lots of eyes here already, some with fuller reviews than myself, so I'll un-assign me. Feel free to call me in again for design discussions, anytime. :)

[jakule]

This PR has a lot of conversations, so let me summarize.
The problem that we're trying to solve is to update User Preferences in the backend. The main issue is that Go has default types, and it is impossible to distinguish between a default value and a value not being set. FieldMaks implemented in this PR, IMO add a lot of complexity.

Currently, on login WebUI already fetches the user preferences. If any changes are made, WebUI can apply those changes to the local copy and send the full version to the backend. This will keep the logic simple.
I know that we don't have very strict rules, but HTTP PUT should that the whole object by definition. It will be less surprising for new users to use our API if we decide to stick to the standard.
Last, we're not using field masks in any other place in our REST API, and I don't think that user preferences are the place where we should deviate from it. I think we should be consistent in our APIs as much as possible.

Let's change the API to take the full user preferences. This solves the default/not set problem. It also removes the merge/patch logic in the backend.

[ibeckermayer]

This PR has a lot of conversations, so let me summarize. The problem that we're trying to solve is to update User Preferences in the backend. The main issue is that Go has default types, and it is impossible to distinguish between a default value and a value not being set. FieldMaks implemented in this PR, IMO add a lot of complexity.

Currently, on login WebUI already fetches the user preferences. If any changes are made, WebUI can apply those changes to the local copy and send the full version to the backend. This will keep the logic simple. I know that we don't have very strict rules, but HTTP PUT should that the whole object by definition. It will be less surprising for new users to use our API if we decide to stick to the standard. Last, we're not using field masks in any other place in our REST API, and I don't think that user preferences are the place where we should deviate from it. I think we should be consistent in our APIs as much as possible.

Let's change the API to take the full user preferences. This solves the default/not set problem. It also removes the merge/patch logic in the backend.

relates to #29234

[jakule]

LGTM

[public-teleport-github-review-bot]

@ryanclark See the table below for backport results.

Branch	Result
branch/v14	Create PR