fix: handle objects in top-level context in playground #6773
Conversation
This file contains 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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
thomasheartman
force-pushed
the
fix/handle-object-values-in-playground-context
branch
from
81ea43d
to
9f2c5ad
Compare
kwasniew
reviewed
Apr 3, 2024
kwasniew
requested changes
Apr 3, 2024
|
Moving the e2e test was also fixed. It wasn't stuck, it just didn't exist before. So without thinking, I created it there because that's where most of the e2e tests live. So thanks for pointing that out! |
thomasheartman
commented
Apr 4, 2024
Comment on lines
+33
to
+78
| test('strips invalid context properties from input before using it', async () => { | ||
| const validData = { | ||
| appName: 'test', | ||
| }; | ||
|
|
||
| const inputContext = { | ||
| invalid: {}, | ||
| ...validData, | ||
| }; | ||
|
|
||
| const { body } = await app.request | ||
| .post('/api/admin/playground/advanced') | ||
| .send({ | ||
| context: inputContext, | ||
| environments: ['production'], | ||
| projects: '*', | ||
| }) | ||
| .expect(200); | ||
|
|
||
| const evaluatedContext = | ||
| body.features[0].environments.production[0].context; | ||
|
|
||
| expect(evaluatedContext).toStrictEqual(validData); | ||
| }); | ||
|
|
||
| test('returns the input context exactly as it came in, even if invalid values have been removed for the evaluation', async () => { | ||
| const invalidData = { | ||
| invalid: {}, | ||
| }; | ||
|
|
||
| const inputContext = { | ||
| ...invalidData, | ||
| appName: 'test', | ||
| }; | ||
|
|
||
| const { body } = await app.request | ||
| .post('/api/admin/playground/advanced') | ||
| .send({ | ||
| context: inputContext, | ||
| environments: ['production'], | ||
| projects: '*', | ||
| }) | ||
| .expect(200); | ||
|
|
||
| expect(body.input.context).toMatchObject(inputContext); | ||
| }); |
There was a problem hiding this comment.
We could put these back into a single test if we want to, but splitting it out into two makes it clearer what they're each testing. Happy to do either.
kwasniew
approved these changes
Apr 5, 2024
thomasheartman
deleted the
fix/handle-object-values-in-playground-context
branch
thomasheartman
added a commit
that referenced
this pull request
Apr 8, 2024
…ings` property (#6784) This PR expands upon #6773 by returning the list of removed properties in the API response. To achieve this, I added a new top-level `warnings` key to the API response and added an `invalidContextProperties` property under it. This is a list with the keys that were removed. ## Discussion points **Should we return the type of each removed key's value?** We could expand upon this by also returning the type that was considered invalid for the property, e.g. `invalidProp: 'object'`. This would give us more information that we could display to the user. However, I'm not sure it's useful? We already return the input as-is, so you can always cross-check. And the only type we allow for non-`properties` top-level properties is `string`. Does it give any useful info? I think if we want to display this in the UI, we might be better off cross-referencing with the input? **Can properties be invalid for any other reason?** As far as I can tell, that's the only reason properties can be invalid for the context. OpenAPI will prevent you from using a type other than string for the context fields we have defined and does not let you add non-string properties to the `properties` object. So all we have to deal with are top-level properties. And as long as they are strings, then they should be valid. **Should we instead infer the diff when creating the model?** In this first approach, I've amended the `clean-context` function to also return the list of context fields it has removed. The downside to this approach is that we need to thread it through a few more hoops. Another approach would be to compare the input context with the context used to evaluate one of the features when we create the view model and derive the missing keys from that. This would probably work in 98 percent of cases. However, if your result contains no flags, then we can't calculate the diff. But maybe that's alright? It would likely be fewer lines of code (but might require additional testing), although picking an environment from feels hacky.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Don't include invalid context properties in the contexts that we evaluate.
This PR removes any non-
propertiesfields that have a non-string value.This prevents the front end from crashing when trying to render an object.
Expect follow-up PRs to include more warnings/diagnostics we can show to the end user to inform them of what fields have been removed and why.