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

16640 fix JSON custom field save nul #16713

Merged
merged 4 commits into from
Aug 16, 2024
Merged

Conversation

arthanson
Copy link
Collaborator

@arthanson arthanson commented Jun 24, 2024

Fixes: #16640

This will fix new saves, doesn't go back and fix old values - could set it to UI editable and edit it to fix. If you have saved multiple times then it will keep escaping so not sure if more dangerous to auto-fix with a validate as JSON and if not valid then revert to ''...

It looks like as it is UI_EDITABLE = FALSE the field isn't getting converted in clean as a JSON field, this works if UI_EDITABLE = True and the field is returned as a dict, if it is not UI editable the fields get returned as a string.

@arthanson arthanson marked this pull request as ready for review June 25, 2024 14:13
@arthanson arthanson requested a review from jeremystretch June 25, 2024 14:13
netbox/netbox/forms/base.py Outdated Show resolved Hide resolved
@FliesLikeABrick
Copy link

Hi all, I am the reported of the original issue. This PR appears to fix the issue for null values only. Editing existing values, or creating a prefix with a default value both lead to escaped strings being placed in the field

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity. It will be closed automatically if no further action is taken.

@github-actions github-actions bot added the pending closure Requires immediate attention to avoid being closed for inactivity label Jul 12, 2024
@jeremystretch
Copy link
Member

@FliesLikeABrick could you please give instructions for reproducing a case which the PR does not address?

@jeremystretch jeremystretch removed the pending closure Requires immediate attention to avoid being closed for inactivity label Jul 12, 2024
@FliesLikeABrick
Copy link

FliesLikeABrick commented Jul 17, 2024

@FliesLikeABrick could you please give instructions for reproducing a case which the PR does not address?

Any non-null custom value is still corrupted via either of the instructions in the original problem report, such as :

Option 2 - corruption on creation:

  1. Create a JSON custom field - set "UI Editable" to "No". Set the default to JSON such as {"key1":"value1"}
  2. Create a prefix
  3. View the prefix, observe that the JSON custom field now shows an escaped string value of "{\"key1\": \"value1\"}"
  4. Edit the prefix and click "save" without making any changes
  5. Observe that the custom value is now escaped again to something like "\"{\\\"key1\\\": \\\"value1\\\"}\""
  6. Repeat edit+save (without making changes) and observe that the prefix keeps getting escaped more and more as a JSON string instead of any valid JSON data structure.

@arthanson arthanson marked this pull request as draft July 26, 2024 15:32
@arthanson arthanson changed the title 16640 fix JSON custom field save nul DRAFT: 16640 fix JSON custom field save nul Jul 26, 2024
@arthanson arthanson changed the title DRAFT: 16640 fix JSON custom field save nul 16640 fix JSON custom field save nul Jul 29, 2024
@arthanson arthanson changed the title 16640 fix JSON custom field save nul DRAFT: 16640 fix JSON custom field save nul Jul 29, 2024
@arthanson arthanson changed the title DRAFT: 16640 fix JSON custom field save nul 16640 fix JSON custom field save nul Jul 29, 2024
@arthanson arthanson marked this pull request as ready for review July 29, 2024 12:49
@arthanson
Copy link
Collaborator Author

@FliesLikeABrick Can you please try the updated PR, I think it should fix the problem for newly created custom fields. It won't change those that are already have incorrect data.

@FliesLikeABrick
Copy link

@arthanson @jeremystretch this PR appears to pass our tests for reproducing the issue, thank you

@FliesLikeABrick
Copy link

Understood that this does not fix existing corrupt/damaged/impacted data - I have written a tool at https://github.com/FliesLikeABrick/netbox_fix_json which can be used to repair impacted data, for anyone else who comes across this.

@rbcollins123
Copy link

Any thoughts on when this will be merged? While we can ignore the human-readability aspect of it, it's becoming progressively more of a problem in some of our real-world uses. We've started having to write in bug-fix wrappers to integration code that reads these fields and recursively decode the values of all JSON fields to get the original data out. Luckily, it's not horrific to fix on the fly as you read it via API, but the longer this bug remains the more of an issue it will be in our object fields over time as people touch/edit objects. Thanks!

@FliesLikeABrick
Copy link

Any thoughts on when this will be merged? While we can ignore the human-readability aspect of it, it's becoming progressively more of a problem in some of our real-world uses. We've started having to write in bug-fix wrappers to integration code that reads these fields and recursively decode the values of all JSON fields to get the original data out. Luckily, it's not horrific to fix on the fly as you read it via API, but the longer this bug remains the more of an issue it will be in our object fields over time as people touch/edit objects. Thanks!

100% agreed, we are having to add fixes to our tooling to catch bad json and repair it; and occasionally running the tool I published to go through and fix our fields of data altogether.

@jeremystretch
Copy link
Member

@rbcollins123 @FliesLikeABrick please understand that NetBox is an extremely active project with very few people actually doing the work. For reference, we have had 73 PRs opened in the past 30 days, and each one of them consumes time.

As a reminder, you always have the option of making changes locally yourself ahead of an official release if there's a truly pressing issue.

@jeremystretch jeremystretch merged commit c457f01 into develop Aug 16, 2024
6 checks passed
@jeremystretch jeremystretch deleted the 16640-custom-json-field branch August 16, 2024 14:30
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI corrupts read-only JSON fields
4 participants