Added reading and writing with JSON for save actions metadata#15159
Added reading and writing with JSON for save actions metadata#15159ZiadAbdElFatah wants to merge 16 commits intoJabRef:mainfrom
Conversation
|
I think now step 1 and step 2 here #10371 (comment) are complete, if so I will move to big part in the issue which is step 3 @koppor.
|
I think its wrong. Because the testing needs to be redone. Based on the custom logic for each preference |
This comment has been minimized.
This comment has been minimized.
Correct me if I am wrong, I know that there will be many tests generated for this feature as mentioned here #10371 (comment)
But I think step 3 must be implemented first
Then the unit tests will be generated for each preference. |
Not for all at once. One after another. An easy one can be done here first to show all the impact. Just reading and writing JSON is OK, but in the cutrent state it does not bring any value to the user. If you feel unable to split down the work in small steps for yourself, maybe this is the wrong issue. |
I do already plan to finish one preference after another, logic, and unit tests for each one. |
|
I added writing and reading for JSON to save actions metadata. |
What is not clear at
You need to be clear when writing "all". "all" matches "Not for all" in my comment.... Same letters!! |
Review Summary by QodoAdd JSON serialization for save actions metadata
WalkthroughsDescription• Add JSON format support for save actions metadata • Implement bidirectional reading/writing of save actions • Maintain backward compatibility with legacy format • Add comprehensive unit tests for JSON parsing Diagramflowchart LR
A["BibDatabaseWriter"] -->|"serialize to JSON"| B["Save Actions JSON"]
C["BibtexParser"] -->|"parse from JSON"| B
B -->|"read/write"| D["MetaData"]
E["Legacy Format"] -->|"backward compatible"| D
File Changes1. jablib/src/main/java/org/jabref/logic/exporter/BibDatabaseWriter.java
|
Code Review by Qodo
1. parseCommentToJson uncaught JsonSyntaxException
|
| Optional<JsonObject> parseCommentToJson(String comment) { | ||
| Gson gson = new Gson(); | ||
| String content = comment.substring(MetaData.META_FLAG_V1.length()); | ||
| return Optional.ofNullable(gson.fromJson(content, JsonObject.class)); | ||
| } |
There was a problem hiding this comment.
1. parsecommenttojson uncaught jsonsyntaxexception 📘 Rule violation ⛯ Reliability
JSON metadata parsing uses gson.fromJson(...) without handling malformed JSON, which can throw a runtime exception and abort import. This violates the requirement for robust, contextual error handling during parsing of external file content.
Agent Prompt
## Issue description
`parseCommentToJson` parses JSON from untrusted file content using `gson.fromJson` without handling malformed JSON, which can throw a runtime exception and crash parsing.
## Issue Context
This is part of BibTeX import logic; invalid/malformed metadata comments should not abort import and should instead be reported as a parser exception/warning with location/context.
## Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/importer/fileformat/BibtexParser.java[415-427]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } else if (comment.startsWith(MetaData.META_FLAG_V1)) { | ||
| parsedJsonMetaData = parseCommentToJson(comment); | ||
| } |
There was a problem hiding this comment.
2. Json meta duplicated on save 🐞 Bug ✓ Correctness
META_FLAG_V1 (@Comment JSON) is parsed but not removed from the raw text buffer, so it can be stored in the first entry’s parsed serialization and later written back verbatim. Since the writer also always emits JSON metadata, saving can duplicate the JSON metadata block in the output file.
Agent Prompt
### Issue description
`META_FLAG_V1` JSON metadata comments are parsed but not removed from the parser’s preserved text buffer. This can cause the JSON `@Comment{jabref-meta-0.1.0 ...}` block to be stored as “user comments before the first entry” and later written back verbatim, while `BibDatabaseWriter` also writes a fresh JSON metadata block — resulting in duplicated JSON metadata after saving.
### Issue Context
Legacy JabRef metadata comments call `dumpTextReadSoFarToString()` to ensure metadata comments are rewritten by JabRef and not kept verbatim in the file. The JSON metadata branch should follow the same strategy.
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/importer/fileformat/BibtexParser.java[372-421]
- jablib/src/main/java/org/jabref/logic/importer/fileformat/BibtexParser.java[335-360]
- jablib/src/main/java/org/jabref/logic/bibtex/BibEntryWriter.java[68-74]
### Suggested approach
1. In the `comment.startsWith(MetaData.META_FLAG_V1)` branch, after parsing (and/or even on parse failure, consistent with how entrytype comments are treated), call `dumpTextReadSoFarToString()` so the comment is not retained as user text.
2. Consider adding a regression test that:
- Parses a file containing only the JSON metadata comment + an entry.
- Saves with `shouldReformatFile() == false` and the entry unchanged.
- Asserts the output contains only one JSON metadata block (not one before the entry plus one in the metadata section).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
I won't be able to continue working on the rest of the metadata until this one is reviewed, to be able to make the best out of the rest of the metadata. Also, I think there will be merge conflicts if I try to work on other metadata before this one is merged. |
koppor
left a comment
There was a problem hiding this comment.
Wrong architecture.
Separate the convertes
We also miss some data models - and model convertion
Look for "Data Transfer Object" implementation pattern in the net.
| + ";}" + OS.NEWLINE | ||
| + OS.NEWLINE | ||
| + "@Comment{jabref-meta-0.1.0" + OS.NEWLINE | ||
| + "{" + OS.NEWLINE | ||
| + " \"saveActions\": {" + OS.NEWLINE | ||
| + " \"state\": true," + OS.NEWLINE | ||
| + " \"title\": [" + OS.NEWLINE | ||
| + " \"lower_case\"" + OS.NEWLINE | ||
| + " ]," + OS.NEWLINE | ||
| + " \"journal\": [" + OS.NEWLINE | ||
| + " \"title_case\"" + OS.NEWLINE | ||
| + " ]," + OS.NEWLINE | ||
| + " \"day\": [" + OS.NEWLINE | ||
| + " \"upper_case\"" + OS.NEWLINE | ||
| + " ]" + OS.NEWLINE | ||
| + " }" + OS.NEWLINE | ||
| + "}" + OS.NEWLINE | ||
| + "}" + OS.NEWLINE; | ||
|
|
There was a problem hiding this comment.
I thought it would be better to follow the old style.
| writeMetaDataJson(metaData); | ||
| } | ||
|
|
||
| private JsonObject serializeSaveActionsToJson(FieldFormatterCleanupActions saveActions) { |
There was a problem hiding this comment.
Wrong place
See preferences how JabRef handles - see org.jabref.logic.preferences.JabRefCliPreferences and the sub preferences
It is a no go to create a monster class BibDatabaseWriter.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ All tests passed ✅🏷️ Commit: 7b1c83a Learn more about TestLens at testlens.app. |
Thanks for providing the correct pattern I studied it well and made the changes. I removed the |
|
Please put the ADR annotations back in. |
) Bumps org.libreoffice:libreoffice from 24.8.4 to 25.2.7. --- updated-dependencies: - dependency-name: org.libreoffice:libreoffice dependency-version: 25.2.7 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
I updated the branch and added the ADR annotations back in, but the same checks still fail. |
|
Will be fixed when #15559 is merged |
Related issues and pull requests
Part of #10371
PR Description
Added reading and writing of save actions metadata using JSON with unit tests.
Now, save actions metadata is written using the old format and the new JSON format, and also, the two formats can be read for save actions.
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)