[shared_preferences] Fix JSON parsing issue with _decodeValue

[aboutmydreams]

[shared_preferences] Fix JSON parsing issue with getAllWithParameters

Description

This PR addresses a JSON parsing issue in the shared_preferences_web package. Previously, the method getAllWithParameters was fetching all keys and decoding them as JSON, which led to parsing errors when the values were not valid JSON formats.

The change introduces a try-catch block around the JSON decoding process to gracefully handle FormatExceptions. If the initial parsing fails, it attempts to parse the value again after enclosing it in double quotes, which is a common fix for strings that represent valid JSON objects. If the second attempt fails, the original string is returned, indicating that the string may not be in a valid JSON format. Additionally, any exceptions are caught, and an empty string is returned while printing the exception details.

Issues Fixed

• Fixes issue where non-JSON formatted strings cause parsing errors when getAllWithParameters is called.

Breaking Change Policy

No changes were made in the [flutter/tests] repo, and thus no migration guide is needed.

Pre-launch Checklist

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I read the [Tree Hygiene] wiki page, which explains my responsibilities.
• I read and followed the [relevant style guides] and ran the auto-formatter.
• I signed the [CLA].
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I listed at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
• I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style].
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is [test-exempt].(needn't do it)
• All existing and new tests are passing.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[stuartmorgan-g]

Thanks for the contribution! You’ve checked boxes in the PR checklist above that are not reflected in this PR, so I’m assuming this is a work in progress and am marking it as a Draft. Please review the checklist, updating the PR as appropriate, and when the checklist reflects the state of the PR as posted please feel free to mark it as ready for review.

[aboutmydreams]

@stuartmorgan I have updated the version number and change log. This PR only involves the capture of a special situation. For example, the browser plug-in does not write localstorage in json format, which will cause a white screen. No need to add new test cases.

[stuartmorgan-g]

For example, the browser plug-in does not write localstorage in json format

What is "the browser plug-in" being referred to here?

It would be helpful if you could file an issue report, with clear reproduction steps for creating the situation this PR is intended to handle.

No need to add new test cases.

If you believe that your PR is exempt from the testing requirements due to falling under one of the cases described in the link provided in the PR checklist and the bot comment, you will need to follow the process described above to request an explicit exemption.

[tarrinneal]

I believe a test showing the case where the previous code failed and the new code doesn't is necessary for this pr.

We don't want to have the code removed since it isn't tested and the case isn't visible to other people making changes in the future.

[aboutmydreams]

What is "the browser plug-in" being referred to here?
It would be helpful if you could file an issue report, with clear reproduction steps for creating the situation this PR is intended to handle.

any browser plugin which write text(not json string) in localstorage

If the value string stored in localstorage does not contain English double quotes, shared_preferences_web will read all key values ​​by default and parse them in json format, resulting in white pages.

If you believe that your PR is exempt from the testing requirements due to falling under one of the cases described in the link provided in the PR checklist and the bot comment, you will need to follow the process described above to request an explicit exemption.
I believe a test showing the case where the previous code failed and the new code doesn't is necessary for this pr. We don't want to have the code removed since it isn't tested and the case isn't visible to other people making changes in the future.

Hi, @stuartmorgan @tarrinneal Since in the current localstorage writing behavior, there are only cases where legal json is written, it seems that I don't know how to add a test for illegal json in the test case.

[stuartmorgan-g]

What is "the browser plug-in" being referred to here?
It would be helpful if you could file an issue report, with clear reproduction steps for creating the situation this PR is intended to handle.

any browser plugin which write text(not json string) in localstorage [...]

This would be good information for the issue I requested; please see also step two of https://github.com/flutter/flutter/wiki/Tree-hygiene#overview

Since in the current localstorage writing behavior, there are only cases where legal json is written, it seems that I don't know how to add a test for illegal json in the test case.

Why can't the test simply write an arbitrary value to localstorage?

[stuartmorgan-g]

@aboutmydreams Are you still planning on updating this based on the feedback above?

[stuartmorgan-g]

The changes to this package should be reverted; only shared_preferences_web is changed, so only it needs version/changelog updates.

[aboutmydreams]

Okay, I reverted it

[aboutmydreams]

Why can't the test simply write an arbitrary value to localstorage?

@stuartmorgan This is possible, but it doesn't make much sense, because this writing method is double quoted by default, so it will not reproduce the error caused by the browser plug-in.

[ditman]

Is this modification needed so shared_preferences_web can read localstorage values written by other programs?

[stuartmorgan-g]

@stuartmorgan This is possible, but it doesn't make much sense, because this writing method is double quoted by default, so it will not reproduce the error caused by the browser plug-in.

I'm not following. If this PR is to handle a case where something is writing an arbitrary value to localstorage in a format the current implementation doesn't understand, why isn't it possible for us to do the same thing in a test?

[aboutmydreams]

Is this modification needed so shared_preferences_web can read localstorage values written by other programs?

@ditman

• Yes, I think it's necessary, for example, when a flutter app is part of an iframe that needs to synchronize its state with localstorage.
• In addition, the current Package operating principle is to obtain and analyze all the key value in LocalStorage. This is why the application is mentioned above

[aboutmydreams]

I'm not following. If this PR is to handle a case where something is writing an arbitrary value to localstorage in a format the current implementation doesn't understand, why isn't it possible for us to do the same thing in a test?

I'm sorry, I'm not sure if I don't express it clearly enough, my English is not very good, so I have used the translation software to express my thoughts

If you still use this Package method to write testing, the data is still written in JSON format. Maybe you can write data from another format by inserting JS, and then use the method of reading after changing in this package to verify, but I don’t know how to execute the writing of JS and guarantee that it will not produce that it will not produce no generate. other problems

[stuartmorgan-g]

If you still use this Package method to write testing, the data is still written in JSON format.

I'm not suggesting that the test uses shared_preferences to write data, I'm suggesting directly writing to localstorage in the test.

[ditman]

@aboutmydreams you should be able to write any value directly to local storage in an integration test, just by using the same js-interop code that the plugin uses, like this:

import 'package:web/web.dart' as html;

...

html.window.localStorage.setItem('someKeyForTesting', 'wrong value');

The snippet above should write a raw (not JSON-encoded) wrong value string into local storage, that then you can try to read from the plugin using the someKeyForTesting key.

[aboutmydreams]

@ditman Happy Chinese New Year, thanks for the tips, I will finish the rest after my vacation is over

[ditman]

@aboutmydreams Happy New Year! Enjoy your time off! 🎊

[stuartmorgan-g]

From triage: @aboutmydreams Are you still planning on returning to this PR?

[stuartmorgan-g]

Thank you for your contribution. I'm going to close this PR for now since there are outstanding comments, just to get this off our PR review queue. Please don't hesitate to submit a new PR if you have the time to address the review comments. Thanks!