fix: incorrect negative value conversion #4316

kyle-ssg · 2024-07-09T10:54:39Z

Thanks for submitting a PR! Please check the boxes below:

I have run pre-commit to check linting
I have added information to docs/ if required so people know about the feature!
I have filled in the "Changes" section below?
I have filled in the "How did you test this code" section below?
I have used a Conventional Commit title for this Pull Request

Changes

Negative ints were being sent as strings to the api when saving a feature.

How did you test this code?

Saved a feature with the value -1

vercel · 2024-07-09T10:54:43Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
flagsmith-frontend-preview	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 21, 2024 9:20am
flagsmith-frontend-staging	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 21, 2024 9:20am

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
docs	⬜️ Ignored (Inspect)	Visit Preview		Aug 21, 2024 9:20am

github-actions · 2024-07-09T10:55:17Z

flagsmith image build and security scan finished ✨

Image	Build Status	Security report
`ghcr.io/flagsmith/flagsmith:pr-4316`	Finished ✅	Results ✅

github-actions · 2024-07-09T10:55:21Z

flagsmith-frontend image build and security scan finished ✨

Image	Build Status	Security report
`ghcr.io/flagsmith/flagsmith-frontend:pr-4316`	Finished ✅	Results ✅

github-actions · 2024-07-09T10:55:22Z

flagsmith-e2e image build finished ✨

Image	Build Status	Security report
`ghcr.io/flagsmith/flagsmith-e2e:pr-4316`	Finished ✅	Skipped

github-actions · 2024-07-09T10:55:25Z

flagsmith-api image build and security scan finished ✨

Image	Build Status	Security report
`ghcr.io/flagsmith/flagsmith-api:pr-4316`	Finished ✅	Results ✅

github-actions · 2024-07-09T10:55:26Z

flagsmith-private-cloud image build and security scan finished ✨

Image	Build Status	Security report
`ghcr.io/flagsmith/flagsmith-private-cloud:pr-4316`	Finished ✅	Results ✅

github-actions · 2024-07-09T10:59:43Z

Uffizzi Preview deployment-55246 was deleted.

question

matthewelwell

Hmm... I tend to agree with this change... but I have a feeling it's a breaking change, right? If people re-save a feature with a negative value it will immediately start returning as an integer in the SDKs.

For reference, I've just checked the production database and we have ~60 such cases.

kyle-ssg · 2024-07-09T15:27:51Z

Hmm yeah, we've fixed this conversion before but I think that only affected people experiencing an issue. It depends on what language SDK they are using (we should maybe track this in headers).

JavaScript will actually work with both, because JavaScript.

rolodato · 2024-07-09T19:20:32Z

For reference, I've just checked the production database and we have ~60 such cases.

@matthewelwell Could you share that dataset/query? I'd like to evaluate our options to move forward based on that.

matthewelwell · 2024-07-12T15:51:24Z

For reference, I've just checked the production database and we have ~60 such cases.

@matthewelwell Could you share that dataset/query? I'd like to evaluate our options to move forward based on that.

I've done this in Slack FYI @rolodato .

rolodato · 2024-08-01T15:16:34Z

I've reviewed the data and there's about 10 organisations, viewable here: https://metabase.flagsmith.com/question/9-features-with-negative-numbers-as-strings. Some of them do have these values set in production so we can't just ship this change as-is, unfortunately.

I can think of a few paths forward:

Abandon this PR and ignore this issue until we can make feature value types explicit, which sidesteps this problem entirely (assuming this is planned/desired)
Add more functionality to this PR to enable users affected by this issue to migrate to the new behaviour at their own pace:
2.1. If a feature is currently a negative number string and a user tries to update it to a negative number, offer a choice to save the value as string (default) or as integer (secondary choice)
2.2. If someone does need to create a new feature with a negative string number, they can use the API manually.
Add more functionality to this PR to display a warning banner when editing a feature with a negative number string, saying that saving changes to the value will change its type to integer, with instructions on how to manually save the value as string if needed.
Set some deadline in the future and notify affected customers that they need to migrate before we ship this PR as-is. CS can create documentation on how SaaS customers need to migrate, how they can tell if they're affected, etc. For on-prem Enterprise customers, we can let them know before they upgrade. On-prem Free users are a bit more tricky to find and notify.

IMO, option 2 is ideal UX until if/when we ship explicit feature value types, but option 3 might be a better effort/reward compromise based on how few SaaS users are actually affected by this. Option 3 is not as much implementation effort and is less risky (no user interaction or manipulating feature values on behalf of users). I'll leave this decision to @kyle-ssg since you'll probably write the code for the actual solution :)

kyle-ssg · 2024-08-14T11:45:34Z

I've been reviewing/attempting 2 and I think it's a big change with lots of code considering the amount of people this will temporarily affect. Especially when this considers change requests and feature versioning.

I'd be inclined to go with 3.

github-actions · 2024-08-14T12:04:14Z

Docker builds report

Image	Build Status	Security report
`ghcr.io/flagsmith/flagsmith-api-test:pr-4316`	Finished ✅	Skipped
`ghcr.io/flagsmith/flagsmith-e2e:pr-4316`	Finished ✅	Skipped
`ghcr.io/flagsmith/flagsmith-api:pr-4316`	Finished ✅	Results ✅
`ghcr.io/flagsmith/flagsmith:pr-4316`	Finished ✅	Results ✅
`ghcr.io/flagsmith/flagsmith-private-cloud:pr-4316`	Finished ✅	Results ✅
`ghcr.io/flagsmith/flagsmith-frontend:pr-4316`	Finished ✅	Results ✅

kyle-ssg · 2024-08-14T12:04:43Z

Ok yep after discussion this is now done,

rolodato · 2024-08-14T20:37:44Z

frontend/web/components/Feature.js

+                Saving this feature will convert its value from a string to a
+                number. If you wish to preserve this value as a string, please
+                save it via our{' '}
+                <a href='https://api.flagsmith.com/api/v1/docs/#/api/api_v1_environments_featurestates_update'>


Minor nit - suggest wording this as please save it using the Admin API for consistency with other docs. LGTM, good stuff 🙏

fix incorrect negative value conversion

820aae8

kyle-ssg requested a review from a team as a code owner July 9, 2024 10:54

kyle-ssg requested review from novakzaballa and removed request for a team July 9, 2024 10:54

github-actions bot added front-end Issue related to the React Front End Dashboard fix labels Jul 9, 2024

kyle-ssg linked an issue Jul 9, 2024 that may be closed by this pull request

Negative number flag values are set to strings after updating #4197

Closed

4 tasks

kyle-ssg requested a review from rolodato July 9, 2024 10:56

vercel bot deployed to Preview – flagsmith-frontend-preview July 9, 2024 10:56 View deployment

vercel bot deployed to Preview – flagsmith-frontend-staging July 9, 2024 10:56 View deployment

matthewelwell previously approved these changes Jul 9, 2024

View reviewed changes

matthewelwell reviewed Jul 9, 2024

View reviewed changes

kyle-ssg assigned rolodato Jul 24, 2024

rolodato assigned kyle-ssg Aug 1, 2024

kyle-ssg added 2 commits August 14, 2024 13:03

Add negative value warning

d3bd37e

Unused import

de8c3c8

github-actions bot removed the fix label Aug 14, 2024

github-actions bot added the fix label Aug 14, 2024

Fix space

1ba7a30

github-actions bot added fix and removed fix labels Aug 14, 2024

vercel bot deployed to Preview – flagsmith-frontend-staging August 14, 2024 12:08 View deployment

vercel bot deployed to Preview – flagsmith-frontend-preview August 14, 2024 12:08 View deployment

Improve copy

6053d7e

github-actions bot added fix and removed fix labels Aug 14, 2024

vercel bot deployed to Preview – flagsmith-frontend-preview August 14, 2024 12:26 View deployment

vercel bot deployed to Preview – flagsmith-frontend-staging August 14, 2024 12:26 View deployment

rolodato approved these changes Aug 14, 2024

View reviewed changes

rolodato reviewed Aug 14, 2024

View reviewed changes

copy

17b45b3

kyle-ssg added this pull request to the merge queue Aug 21, 2024

github-actions bot added fix and removed fix labels Aug 21, 2024

Merged via the queue into main with commit 2931cdf Aug 21, 2024
17 of 20 checks passed

kyle-ssg deleted the fix/incorrect-negative-number-conversion branch August 21, 2024 09:19

flagsmithdev mentioned this pull request Aug 20, 2024

chore(main): release 2.138.0 #4521

Merged

vercel bot deployed to Preview – flagsmith-frontend-staging August 21, 2024 09:20 View deployment

vercel bot deployed to Preview – flagsmith-frontend-preview August 21, 2024 09:20 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: incorrect negative value conversion #4316

fix: incorrect negative value conversion #4316

kyle-ssg commented Jul 9, 2024

vercel bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading

matthewelwell left a comment •

edited

Loading

kyle-ssg commented Jul 9, 2024

rolodato commented Jul 9, 2024

matthewelwell commented Jul 12, 2024

rolodato commented Aug 1, 2024

kyle-ssg commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading

kyle-ssg commented Aug 14, 2024

rolodato Aug 14, 2024

fix: incorrect negative value conversion #4316

fix: incorrect negative value conversion #4316

Conversation

kyle-ssg commented Jul 9, 2024

Changes

How did you test this code?

vercel bot commented Jul 9, 2024 • edited Loading

github-actions bot commented Jul 9, 2024 • edited Loading

github-actions bot commented Jul 9, 2024 • edited Loading

github-actions bot commented Jul 9, 2024 • edited Loading

github-actions bot commented Jul 9, 2024 • edited Loading

github-actions bot commented Jul 9, 2024 • edited Loading

github-actions bot commented Jul 9, 2024 • edited Loading

matthewelwell left a comment • edited Loading

Choose a reason for hiding this comment

kyle-ssg commented Jul 9, 2024

rolodato commented Jul 9, 2024

matthewelwell commented Jul 12, 2024

rolodato commented Aug 1, 2024

kyle-ssg commented Aug 14, 2024 • edited Loading

github-actions bot commented Aug 14, 2024 • edited Loading

Docker builds report

kyle-ssg commented Aug 14, 2024

rolodato Aug 14, 2024

Choose a reason for hiding this comment

vercel bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading

matthewelwell left a comment •

edited

Loading

kyle-ssg commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading