Fix panic when fetching user preferences#28751
Merged
Conversation
michellescripts
approved these changes
Jul 6, 2023
zmb3
reviewed
Jul 6, 2023
Contributor
Author
There was a problem hiding this comment.
I know 🙈 But IMO doesn't make sense to have the default values already specified above and keep adding nested ifs to set the same values instead of using reflection here. I was hoping to find something in proto package, but proto.Range() only iterates over set values, and proto.Merge() merges collections, so it won't work here.
Contributor
There was a problem hiding this comment.
But we do want to iterate only over fields that are set in src, right? So it seems Range() should do the job?. OTOH Range is pretty close to reflection either way so.. 🤷
Contributor
Author
There was a problem hiding this comment.
Range() iterates only over non-nil fields, and these are exactly the fields that I need.
Contributor
There was a problem hiding this comment.
So.. it can be used? AFAICT the flow is as follow:
- find all fields that are set (non-default) in src
- check if field is set in dst
- if not set it to value from src
Rangecan give you the first step, right?
Contributor
Author
There was a problem hiding this comment.
@zmb3 @probakowski I've removed the reflection explicit from our code.
jakule
force-pushed
the
jakule/fix-panic-user-prefs
branch
from
1756cb2 to
0c4f943
Compare
This commit updates the approach for comparing equality in user preferences test. It introduces the use of the "go-cmp" library which provides more flexibility in handling comparison of struct elements, thus helping to catch any unexpected changes that could be overlooked with the standard equality checking. We also revised the logic to handle default preferences by overwriting values for better efficiency and readability, also removing unnecessary checking and merging of values.
jakule
force-pushed
the
jakule/fix-panic-user-prefs
branch
from
0c4f943 to
0d6bdc5
Compare
zmb3
approved these changes
Jul 11, 2023
public-teleport-github-review-bot
Bot
removed the request for review
from atburke
tigrato
approved these changes
Jul 11, 2023
| dest.Theme = src.Theme | ||
| } | ||
| // overwriteValues overwrites the values in dst with the values in src. | ||
| func overwriteValues(dst, src protoreflect.ProtoMessage) error { |
Contributor
There was a problem hiding this comment.
can you please add a comment mentioning that it only works because proto.range only visits non-nil/non-empty fields?
It would become clear to anyone using it how it works.
Added detailed comments to better explain the overwriteValues function in userpreferences.go file. The comments clarify how the function uses proto.Ranges to iterate over fields and only overwrite non-nil/empty fields.
probakowski
approved these changes
Jul 11, 2023
michellescripts
pushed a commit
that referenced
this pull request
Jul 21, 2023
* Fix panic when fetching user preferences Closes #28740 * Prevent overwriting mismatched types in user preferences * Add error handling to user preferences service * GCI imports * Refactor user preferences test and logic This commit updates the approach for comparing equality in user preferences test. It introduces the use of the "go-cmp" library which provides more flexibility in handling comparison of struct elements, thus helping to catch any unexpected changes that could be overlooked with the standard equality checking. We also revised the logic to handle default preferences by overwriting values for better efficiency and readability, also removing unnecessary checking and merging of values. * Add comments to overwriteValues method in userpreferences.go Added detailed comments to better explain the overwriteValues function in userpreferences.go file. The comments clarify how the function uses proto.Ranges to iterate over fields and only overwrite non-nil/empty fields. * Apply some magic to preferences test * Change the import to avoid go.mod changed and match our other imports
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Jul 21, 2023
* Add onboarding questionnaire components (#27977) * Posthog event for onboarding questionnaire submit (#28328) * update protos * create Posthog event for onboarding questionnaire submit * set onboarding survey on user preferences (#28530) * Fix panic when fetching user preferences (#28751) * Fix panic when fetching user preferences Closes #28740 * Prevent overwriting mismatched types in user preferences * Add error handling to user preferences service * GCI imports * Refactor user preferences test and logic This commit updates the approach for comparing equality in user preferences test. It introduces the use of the "go-cmp" library which provides more flexibility in handling comparison of struct elements, thus helping to catch any unexpected changes that could be overlooked with the standard equality checking. We also revised the logic to handle default preferences by overwriting values for better efficiency and readability, also removing unnecessary checking and merging of values. * Add comments to overwriteValues method in userpreferences.go Added detailed comments to better explain the overwriteValues function in userpreferences.go file. The comments clarify how the function uses proto.Ranges to iterate over fields and only overwrite non-nil/empty fields. * Apply some magic to preferences test * Change the import to avoid go.mod changed and match our other imports * Onboarding changes to support e survey (#28981) * Display a survey during onboarding - Adds user context wrapper to the Welcome flow in order to update preferences - Sets selected resources on user preferences * imports & describes * named enums * remove onboard survey from oss - will be moved to /e - export shared types * refactor welcome - move new credential to welcome prop * refactor new credentails - container passes questionnaire to component * remove survey ls dispatch * pr feedback * Revert "pr feedback" This reverts commit 060f34fb60242856f233df6a1006c5192ea321fb. --------- Co-authored-by: Jakub Nyckowski <jakub.nyckowski@goteleport.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #28740