Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feature] allow user custom max depth #155

Closed
wants to merge 1 commit into from

Conversation

shoothzj
Copy link
Collaborator

@shoothzj shoothzj commented Jul 1, 2023

This pr currently is a draft. Only has public api, but still missing implementation and tests.

@shoothzj
Copy link
Collaborator Author

shoothzj commented Jul 1, 2023

@UrielCh or we can deprecate the permissiveMode constructor, use ParserOption instead, WDYT?

@@ -103,7 +107,7 @@ public JSONParserBase(int permissiveMode) {
this.useHiPrecisionFloat = (permissiveMode & JSONParser.USE_HI_PRECISION_FLOAT) > 0;
this.checkTaillingData = (permissiveMode & (JSONParser.ACCEPT_TAILLING_DATA
| JSONParser.ACCEPT_TAILLING_SPACE)) != (JSONParser.ACCEPT_TAILLING_DATA
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my screen is not large enough to read this line ...
may I get a human translation for this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@UrielCh It's a style miss change. I will revert it.

* See the License for the specific language governing permissions and
* limitations under the License.
*/
public class ParserOptions {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding a flag to remove the limit can be enough?

adding and extra ParserOptions only for that, is... overkill ?

and this PR looks not to use maxDepth.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@UrielCh Yes, as I said before, it's only a draft about external API changes.

@shoothzj
Copy link
Collaborator Author

shoothzj commented Jul 5, 2023

@UrielCh We may not anticipate new requirements in the future, and it may be better to add an optional maximum value. What if the users reach the 1000 limit? We will have many flags that are difficult to maintain.

adding and extra ParserOptions only for that, is... overkill ?

We may add other flags as boolean variables in ParserOptions, flags or Options?

@UrielCh
Copy link
Contributor

UrielCh commented Jul 7, 2023

Objects with a depth higher than 400 are not man-made and can be 1000 or 2000 level depth.

For users using super-depth JSON objects, just drop the limit with a flag.

Are you agree with this point?

@shoothzj

Json-smart is in a stable state and should not introduce big changes.

Or, if you have some bigger plan for the project, suggest a new roadmap that will use this extra option parameter.

@shoothzj
Copy link
Collaborator Author

shoothzj commented Jul 7, 2023

@UrielCh In consideration of stability, I agree to add a flag to remove this restriction. However, I believe that it's more common in Java to use struct configurations rather than flags. We could plan to implement this in future versions.

@shoothzj
Copy link
Collaborator Author

shoothzj commented Jul 7, 2023

@UrielCh I created a draft #156 PTAL

@UrielCh
Copy link
Contributor

UrielCh commented Jul 7, 2023

I did not use configuration in json-smart for ... allmost 15 years, so adding a configuration for a single parameter, when a simple flag to disable the new limitation will do the job... no...
except if you plan to add a bunch of new feature that will use this configuration object.

@UrielCh
Copy link
Contributor

UrielCh commented Jul 7, 2023

PR commented.

@shoothzj shoothzj closed this Jul 8, 2023
@shoothzj shoothzj deleted the allow-custom-max-depth branch March 12, 2024 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants