More code cleanup and refactoring #245
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications across various classes and interfaces within the Metaschema framework. Key alterations include changing the visibility of static fields in Changes
🪧 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 (
|
…operations to the atomic item implementations. This creates a cleaner isolation between the type adapters and Metapath items.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (114)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/StringItemImpl.java (1)
13-15: Consider enhancing class documentation.While the current documentation is clear, consider adding:
- Reference to the implemented interface
- Relationship with the parent class
- API version information
/** * An implementation of a Metapath atomic item containing a text data value. + * + * This class implements {@code IStringItem} and extends {@code AbstractStringItem} + * to provide concrete string value handling functionality. + * + * @since 1.0.0 */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AnyUriItemImpl.java (2)
15-17: Consider enhancing class-level documentation.While the current documentation is clear, it could be more comprehensive by:
- Specifying which interface this class implements
- Including example usage
- Explaining its relationship with
AbstractUriItemExample enhancement:
/** * An implementation of a Metapath atomic item containing a URI data value. + * This class extends {@link AbstractUriItem} to provide concrete implementation + * for URI-based atomic items in the Metapath system. + * + * <p>Example usage: + * <pre> + * URI uri = URI.create("http://example.com"); + * AnyUriItemImpl item = new AnyUriItemImpl(uri); + * </pre> */
Line range hint
31-34: Add documentation for overridden method.Consider adding method-level documentation for consistency, even though it's an override.
+ /** + * {@inheritDoc} + * + * @return the URI adapter singleton instance from {@link MetaschemaDataTypeProvider} + */ @Override public UriAdapter getJavaTypeAdapter() { return MetaschemaDataTypeProvider.URI; }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/IntegerItemImpl.java (1)
15-19: Consider enhancing class documentation.While the current Javadoc describes the basic purpose, it could be more informative by:
- Mentioning the relationship with
AbstractIntegerItem- Documenting thread-safety guarantees
- Adding
@sincetag to track API versioningExample enhancement:
/** * An implementation of a Metapath atomic item containing an integer data value. + * This implementation extends {@link AbstractIntegerItem} and provides a thread-safe + * wrapper around {@link BigInteger} values. + * + * @since 1.0.0 */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/TokenItemImpl.java (2)
14-17: Consider adding @SInCE tag to the class JavadocWhile the documentation is clear and descriptive, adding a @SInCE tag would help track API versioning.
/** * An implementation of a Metapath atomic item containing a text token data * value. + * + * @since 1.0.0 */
Line range hint
32-35: Add Javadoc for getJavaTypeAdapter methodConsider adding method-level documentation to maintain consistency with the rest of the codebase.
+ /** + * @return the token adapter instance for handling token data type conversions + */ @Override public TokenAdapter getJavaTypeAdapter() { return MetaschemaDataTypeProvider.TOKEN; }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/HostnameItemImpl.java (1)
14-16: Consider enhancing Javadoc with version informationThe Javadoc is clear but could benefit from additional metadata.
Consider adding:
/** * An implementation of a Metapath atomic item containing a hostname data value. + * + * @since [version] */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/EmailAddressItemImpl.java (2)
14-17: Consider enhancing Javadoc with additional details.While the current documentation is clear, consider adding:
@sincetag to track API versioning- Details about email address format/validation requirements
- Example usage
/** * An implementation of a Metapath atomic item containing an email address data * value. + * + * @since 1.0.0 + * @see EmailAddressAdapter for email address format and validation rules + * @example + * EmailAddressItem email = new EmailAddressItemImpl("[email protected]"); */
22-30: Consider adding email format validation in constructor.While the constructor properly handles null safety with @nonnull, consider validating the email format at construction time to fail fast.
public EmailAddressItemImpl(@NonNull String value) { + if (!EmailAddressAdapter.isValidEmailAddress(value)) { + throw new IllegalArgumentException("Invalid email address format: " + value); + } super(value); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/NcNameItemImpl.java (3)
6-11: LGTM! Good architectural improvementMoving the implementation to an
implpackage follows the best practice of separating interfaces from their implementations, improving modularity and maintainability.
14-18: Enhance deprecation documentation with migration guidanceWhile the class documentation is clear, consider adding migration guidance in the Javadoc to help users transition away from this deprecated class.
/** * An implementation of a Metapath atomic item containing a non-colonized name * data value. + * + * @deprecated Since 0.7.0, will be removed in a future version. + * Use {@link AlternativeClass} instead. */
Line range hint
23-37: Consider adding explicit null check despite @nonnull annotationWhile the @nonnull annotation provides compile-time safety, consider adding a runtime check for additional robustness:
public NcNameItemImpl(@NonNull String value) { + Objects.requireNonNull(value, "value"); super(value); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/UriReferenceItemImpl.java (2)
16-19: Consider enhancing class documentation.While the current documentation is clear, it could be more comprehensive by including:
- Examples of valid URI references
- Any specific handling or validation rules
- Relationship with
AbstractUriItem
Line range hint
34-37: Consider adding Javadoc for the getJavaTypeAdapter method.While the implementation is correct, adding method documentation would improve code maintainability by explaining:
- The purpose of the adapter
- The relationship with MetaschemaDataTypeProvider
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateWithTimeZoneItemImpl.java (2)
15-18: Consider enhancing class documentationWhile the current documentation is clear, it could be more comprehensive by including:
- Details about timezone handling
- Examples of valid date formats
- Related classes or interfaces
/** * An implementation of a Metapath atomic item containing a date data value that * has a required timezone. + * + * <p>This class handles dates with explicit timezone information using ZonedDateTime. + * The timezone is preserved as specified in the input and is significant for + * date/time operations and comparisons. + * + * @see java.time.ZonedDateTime + * @see AbstractDateItem */
Line range hint
32-41: Add method-level documentationConsider adding Javadoc for the overridden methods to improve code maintainability.
+ /** + * @return the stored ZonedDateTime value + */ @Override public ZonedDateTime asZonedDateTime() { return getValue(); } + /** + * @return the DateWithTZAdapter instance from MetaschemaDataTypeProvider + */ @Override public DateWithTZAdapter getJavaTypeAdapter() { return MetaschemaDataTypeProvider.DATE_WITH_TZ; }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/PositiveIntegerItemImpl.java (1)
16-22: Consider enhancing class documentationWhile the current Javadoc is good, it could be more detailed about the constraints of positive integers (e.g., value > 0).
Suggested enhancement:
/** * An implementation of a Metapath atomic item containing a positive integer * data value. + * + * This implementation ensures that the wrapped value is always greater than zero, + * as per the positive integer constraint. */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateTimeWithTimeZoneItemImpl.java (2)
15-18: Consider enhancing class documentation.While the current documentation is clear, it could be more helpful by including:
- Details about timezone handling behavior
- Example usage
- Related classes/interfaces
Example enhancement:
/** * An implementation of a Metapath atomic item containing a date/time data value * that has a required timezone. + * + * <p>This implementation ensures that all date/time values maintain timezone information, + * making it suitable for scenarios requiring precise temporal specifications across + * different timezones. It works in conjunction with {@link DateTimeWithTZAdapter} + * for type conversion operations. + * + * <p>Example usage: + * <pre> + * ZonedDateTime dt = ZonedDateTime.now(); + * DateTimeWithTimeZoneItemImpl item = new DateTimeWithTimeZoneItemImpl(dt); + * </pre> + * + * @see IDateTimeItem + * @see DateTimeWithTZAdapter */
22-30: Enhance constructor documentation with null handling details.The constructor documentation should explicitly mention the @nonnull constraint.
/** * Construct a new item with the provided {@code value}. * * @param value - * the value to wrap + * the value to wrap (must not be null) + * @throws NullPointerException if value is null */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/NonNegativeIntegerItemImpl.java (3)
6-14: LGTM! Package structure follows best practicesThe move to the
.implpackage properly separates implementation from interface, improving the overall architecture.
16-19: Consider enhancing class documentationWhile the current documentation is good, consider adding:
- A note about implementing
INonNegativeIntegerItem- The relationship with
AbstractIntegerItem- Any constraints or validation on the non-negative values
/** * An implementation of a Metapath atomic item containing a non-negative integer * data value. + * + * <p>This class extends {@link AbstractIntegerItem} and implements {@link INonNegativeIntegerItem} + * to provide a concrete implementation for handling non-negative integer values in the Metapath + * system. The class ensures all values are non-negative through validation in the constructor. */
33-36: Consider adding method documentationAdd Javadoc to explain the purpose of overriding this method and its relationship with the type system.
+ /** + * {@inheritDoc} + * + * @return the singleton instance of {@link NonNegativeIntegerAdapter} from + * {@link MetaschemaDataTypeProvider} + */ @Override public NonNegativeIntegerAdapter getJavaTypeAdapter() { return MetaschemaDataTypeProvider.NON_NEGATIVE_INTEGER; }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateWithoutTimeZoneItemImpl.java (3)
6-6: Good architectural decision moving implementation to.implpackage!This change improves encapsulation by clearly separating implementation details from the API, following Java best practices.
16-19: Consider enhancing class-level Javadoc.While the current documentation is clear, consider adding:
@sincetag to track API versioning- Example usage to help developers understand how to use this implementation
Example enhancement:
/** * An implementation of a Metapath atomic item containing a date data value that * may not have an explicit timezone. + * + * @since 1.0.0 + * + * <pre> + * Example: + * Date date = new Date(...); + * DateWithoutTimeZoneItemImpl item = new DateWithoutTimeZoneItemImpl(date); + * ZonedDateTime zdt = item.asZonedDateTime(); + * </pre> */
23-28: Document @throws for null parameter.The @nonnull annotation suggests that null values are not allowed, but this isn't documented. Consider adding a @throws tag to make this explicit.
/** * Construct a new item with the provided {@code value}. * * @param value * the value to wrap + * @throws NullPointerException if {@code value} is null */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateTimeWithoutTimeZoneItemImpl.java (3)
Line range hint
6-14: LGTM! Good package organization.Moving the implementation to a dedicated
.implpackage improves separation of concerns and follows good architectural practices.
16-19: Consider enhancing class-level documentation.While the current documentation is clear, it could be more specific about:
- The behavior when timezone is missing
- The relationship with AbstractDateTimeItem
- Version information
Consider adding:
/** * An implementation of a Metapath atomic item containing a date/time data value * that may not have an explicit timezone. + * + * When no timezone is specified, the system default timezone is used as per + * AbstractDateTimeItem behavior. + * + * @since 1.0.0 */
Missing Javadoc Comments and Inadequate Timezone Handling Tests
DateTimeWithoutTimeZoneItemImpl.java:
- Missing Javadoc before overridden methods at lines 33 and 38.
- Timezone Handling Tests:
- No comprehensive tests found for timezone conversions or edge cases.
🔗 Analysis chain
Line range hint
20-43: Add documentation for overridden methods and verify timezone handling.The implementation looks correct, but consider:
- Adding
@OverrideJavadoc forasZonedDateTime()andgetJavaTypeAdapter()- Verifying timezone conversion behavior for edge cases
Let's check for consistent timezone handling across the codebase:
Consider adding:
@Override + /** + * Converts the stored DateTime value to a ZonedDateTime. + * + * @return the date/time value as a ZonedDateTime + */ public ZonedDateTime asZonedDateTime() { return getValue().getValue(); } @Override + /** + * @return the DateTimeAdapter instance for type conversion + */ public DateTimeAdapter getJavaTypeAdapter() { return MetaschemaDataTypeProvider.DATE_TIME; }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for timezone-related patterns in AbstractDateTimeItem and related classes echo "Checking timezone handling implementation..." rg -A 5 "ZonedDateTime|TimeZone|ZoneId" --type java # Look for potential timezone conversion tests echo "Checking timezone-related tests..." fd -e java -E build --exec grep -l "test.*[Tt]ime[Zz]one" {} \;Length of output: 72525
Script:
#!/bin/bash # Check for overridden methods without Javadoc in DateTimeWithoutTimeZoneItemImpl.java echo "Checking for @Override methods without preceding Javadoc in DateTimeWithoutTimeZoneItemImpl.java..." rg -n '@Override' core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateTimeWithoutTimeZoneItemImpl.java | while read -r line; do line_num=$(echo "$line" | cut -d: -f1) # Get the line before @Override prev_line=$(sed -n "$((line_num -1))p" core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateTimeWithoutTimeZoneItemImpl.java) # Check if the previous line starts with /** if ! echo "$prev_line" | grep -q '/\*\*'; then echo "Missing Javadoc before line $line_num: $line" fi done # Search for timezone handling tests in the codebase echo "Searching for timezone handling tests in the codebase..." rg -t java 'test.*[Tt]ime[Zz]one' .Length of output: 1933
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupDataTypeProvider.java (1)
18-31: Consider using factory methods instead of public static fields.While the implementation is well-documented and follows naming conventions, exposing static instances directly might limit flexibility. Consider using factory methods to:
- Better control instance lifecycle
- Allow for potential future initialization logic
- Maintain better encapsulation
Example refactor:
- @NonNull - public static final MarkupLineAdapter MARKUP_LINE = new MarkupLineAdapter(); - - @NonNull - public static final MarkupMultilineAdapter MARKUP_MULTILINE = new MarkupMultilineAdapter(); + private static final MarkupLineAdapter MARKUP_LINE_INSTANCE = new MarkupLineAdapter(); + private static final MarkupMultilineAdapter MARKUP_MULTILINE_INSTANCE = new MarkupMultilineAdapter(); + + /** + * Gets the Metaschema markup-line data type instance. + * + * @return the markup-line data type instance + * @see <a href="https://pages.nist.gov/metaschema/specification/datatypes/#markup-line">markup-line</a> + */ + @NonNull + public static MarkupLineAdapter getMarkupLine() { + return MARKUP_LINE_INSTANCE; + } + + /** + * Gets the Metaschema markup-multiline data type instance. + * + * @return the markup-multiline data type instance + * @see <a href="https://pages.nist.gov/metaschema/specification/datatypes/#markup-multiline">markup-multiline</a> + */ + @NonNull + public static MarkupMultilineAdapter getMarkupMultiline() { + return MARKUP_MULTILINE_INSTANCE; + }core/src/main/java-templates/gov/nist/secauto/metaschema/core/MetaschemaJavaVersion.java (1)
6-8: Enhance class documentation for better clarity.While the current documentation is clear, it could be more descriptive about the specific version information provided and its implementation of IVersionInfo.
Consider expanding the Javadoc:
/** * Provides version information for this library. + * + * This class implements {@link IVersionInfo} to expose build-time metadata including + * version numbers, build timestamps, and Git repository information. */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/IPv4AddressItemImpl.java (2)
16-19: Consider enhancing Javadoc with @SInCE tag and example usage.While the current documentation is clear, adding version information and usage example would make it more helpful for developers.
/** * An implementation of a Metapath atomic item containing an IPv4 address data * value. + * + * @since 1.0.0 + * + * <pre> + * IPv4Address addr = new IPv4Address("192.168.1.1"); + * IPv4AddressItemImpl item = new IPv4AddressItemImpl(addr); + * </pre> */
24-32: Consider adding explicit null check for runtime safety.While the @nonnull annotation helps at compile-time, an explicit null check would provide better runtime safety and clearer error messages.
public IPv4AddressItemImpl(@NonNull IPv4Address value) { + if (value == null) { + throw new IllegalArgumentException("IPv4Address value cannot be null"); + } super(value); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/IPv6AddressItemImpl.java (3)
16-19: Consider enhancing class documentation.While the current documentation is clear, consider adding:
- Interface implementation details
- Thread-safety characteristics
- Example usage
/** * An implementation of a Metapath atomic item containing an IPv6 address data * value. + * + * <p>This class implements {@link IIPv6AddressItem} and is immutable and thread-safe. + * + * <p>Example usage: + * <pre>{@code + * IPv6Address address = IPv6Address.getByName("2001:db8::1"); + * IPv6AddressItemImpl item = new IPv6AddressItemImpl(address); + * }</pre> */
24-32: Consider adding explicit null check in constructor.While the @nonnull annotation is present, adding an explicit null check would provide better runtime safety and clearer error messages.
public IPv6AddressItemImpl(@NonNull IPv6Address value) { + Objects.requireNonNull(value, "value must not be null"); super(value); }
Inconsistent Usage of IPv6AddressAdapter Detected
Instances of
new IPv6AddressAdapter()found in test files. Please use the singleton adapter fromMetaschemaDataTypeProviderto ensure consistency across the codebase.
- core/src/test/java/gov/nist/secauto/metaschema/core/datatype/adapter/IPv6AddressAdapterTest.java
🔗 Analysis chain
Line range hint
34-43: LGTM! Methods are correctly implemented.The implementation is clean and efficient. The use of the singleton adapter pattern through MetaschemaDataTypeProvider is appropriate.
Let's verify consistent adapter usage across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of IPv6AddressAdapter # Expected: All IPv6Address handling should use the singleton adapter # Search for IPv6AddressAdapter usage rg -A 2 "IPv6AddressAdapter" # Search for potential direct instantiation ast-grep --pattern 'new IPv6AddressAdapter'Length of output: 3285
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/MarkupLineItemImpl.java (2)
16-19: Consider enhancing the class documentation.While the current documentation is good, consider adding:
@sincetag to track API version- Example usage
- Reference to
IMarkupIteminterface/** * An implementation of a Metapath atomic item representing a single line Markup * data value. + * + * @since 1.0.0 + * + * @see IMarkupItem + * + * @example + * MarkupLine markup = new MarkupLine("Some markup text"); + * MarkupLineItemImpl item = new MarkupLineItemImpl(markup); */
Line range hint
34-43: Add method-level documentation and @OverRide annotations.While the implementations are correct, consider adding method documentation and ensuring all overridden methods are properly annotated.
+ /** + * {@inheritDoc} + */ @Override public MarkupLine asMarkup() { return getValue(); } + /** + * {@inheritDoc} + */ @Override public MarkupLineAdapter getJavaTypeAdapter() { return MarkupDataTypeProvider.MARKUP_LINE; }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/MarkupMultiLineItemImpl.java (1)
Line range hint
34-43: Consider adding method-level Javadoc.While the implementations are correct, consider adding method-level Javadoc to document the contract these methods fulfill from the IMarkupItem interface.
+ /** + * {@inheritDoc} + */ @Override public MarkupMultiline asMarkup() { return getValue(); } + /** + * {@inheritDoc} + */ @Override public MarkupMultilineAdapter getJavaTypeAdapter() { return MarkupDataTypeProvider.MARKUP_MULTILINE; }core/src/main/java-templates/gov/nist/secauto/metaschema/core/model/MetaschemaVersion.java (1)
7-9: Consider enhancing class documentationWhile the current documentation provides basic information, consider adding:
- A note about implementing
IVersionInfo- Details about the specific version information provided
@sincetag for API documentation/** * Provides version information for the underlying Metaschema implementation used by this library. + * + * This class implements {@link IVersionInfo} to provide build-time information including version, + * timestamp, and git metadata about the Metaschema implementation. + * + * @since 1.0.0 */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractDateItem.java (1)
13-19: Consider adding @SInCE tag to class documentationWhile the documentation is clear and well-structured, consider adding a
@sincetag to indicate when this implementation was introduced. This helps with API versioning and maintenance.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractDateTimeItem.java (2)
13-19: Consider adding @SInCE tag for API versioning.The Javadoc is well-written and follows conventions. Consider adding a @SInCE tag to help track API versions.
/** * An abstract implementation of a Metapath atomic item containing a date/time * data value. * * @param <TYPE> * the Java type of the wrapped value + * @since 1.0.0 */
52-52: Consider using pattern matching for instanceof (Java 16+).The equals implementation is correct, but could be modernized using pattern matching if the project targets Java 16 or higher.
- || obj instanceof IDateTimeItem && compareTo((IDateTimeItem) obj) == 0; + || obj instanceof IDateTimeItem other && compareTo(other) == 0;core/src/test/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnCeilingTest.java (1)
29-30: Consider adding more test cases for better coverageThe current test cases correctly verify ceiling operation with integer inputs, but consider adding:
- Test cases with decimal inputs (e.g.,
decimal("10.1"))- Edge cases:
- Zero:
decimal("0.0")- Large numbers near type limits
- Numbers very close to integers (e.g.,
decimal("10.999999"))Example additional test cases:
private static Stream<Arguments> provideValues() { return Stream.of( Arguments.of(integer(11), decimal("10.5")), Arguments.of(integer(-10), decimal("-10.5")) + Arguments.of(integer(11), decimal("10.1")), + Arguments.of(integer(0), decimal("0.0")), + Arguments.of(integer(1), decimal("0.999999")) ); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractDecimalItem.java (3)
6-6: LGTM! Package restructuring improves separation of concernsThe move to the
implpackage better separates implementation details from the public API, following good architectural practices.Also applies to: 8-9
14-20: Consider adding @SInCE tag to the class documentationWhile the Javadoc is well-written, consider adding a
@sincetag to indicate when this implementation was introduced. This helps with API versioning and maintenance./** * An abstract implementation of a Metapath atomic item containing a decimal * data value. * * @param <TYPE> * the Java type of the wrapped value + * @since 1.0.0 */
56-57: Consider extracting the comparison logic for better readabilityThe equals method contains complex casting and nested method calls. Consider extracting this logic to improve readability.
@Override public boolean equals(Object obj) { - return this == obj || - obj instanceof AbstractDecimalItem.MapKey - && getKey().asDecimal().equals(((AbstractDecimalItem<?>.MapKey) obj).getKey().asDecimal()); + if (this == obj) { + return true; + } + if (!(obj instanceof AbstractDecimalItem.MapKey)) { + return false; + } + AbstractDecimalItem<?>.MapKey other = (AbstractDecimalItem<?>.MapKey) obj; + return getKey().asDecimal().equals(other.getKey().asDecimal()); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractUriItem.java (2)
17-20: Consider adding @SInCE tag for API versioningThe JavaDoc is clear and well-written. Consider adding a
@sincetag to indicate when this class was introduced, which helps with API versioning./** * An abstract implementation of a Metapath atomic item containing a URI data * value. + * + * @since 1.0.0 */
21-23: LGTM! Well-designed class with proper encapsulationThe class follows good object-oriented principles:
- Proper inheritance hierarchy
- Thread-safe implementation through immutability
- Consistent use of null-safety annotations
Consider documenting the thread-safety guarantees in the class JavaDoc for better API documentation.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractIntegerItem.java (1)
16-19: Consider adding @SInCE tag to the class documentationThe Javadoc clearly describes the class purpose. Consider adding a
@sincetag to track API versioning, which is particularly useful for abstract classes that are part of the public API./** * An abstract implementation of a Metapath atomic item containing an integer * data value. + * + * @since 1.0.0 */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractTemporalItem.java (1)
Line range hint
47-63: Consider using Objects.equals for better null safety.While the equals implementation is correct, it could be made more robust by using
Objects.equals(). This would provide additional null safety and follow Java best practices.@SuppressWarnings("PMD.OnlyOneReturn") @Override public boolean equals(Object obj) { if (this == obj) { return true; } if (!(obj instanceof AbstractTemporalItem.MapKey)) { return false; } AbstractTemporalItem<?>.MapKey other = (AbstractTemporalItem<?>.MapKey) obj; - return getKey().compareTo(other.getKey()) == 0; + return Objects.equals(getKey(), other.getKey()); }core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/UriAdapter.java (5)
22-26: Enhance class documentation with additional detailsWhile the documentation provides a good basic description, consider adding:
@sincetag to track when this adapter was introduced@seetags referencing related classes (e.g.,IAnyUriItem)- Information about how invalid URIs are handled
/** * Support for the Metaschema <a href= * "https://pages.nist.gov/metaschema/specification/datatypes/#uri">uri</a> data * type. + * + * @since [version] + * @see IAnyUriItem + * @see URI + * + * @throws IllegalArgumentException if the provided string cannot be parsed as a valid URI */
Line range hint
45-48: Add exception handling in parse methodThe
URI.create()method can throwIllegalArgumentException. This should be documented and potentially wrapped in a more specific exception type.@Override public URI parse(String value) { - return URI.create(value); + try { + return URI.create(value); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Invalid URI format: " + value, e); + } }
Line range hint
50-54: Improve type safety in copy methodThe current implementation uses an unsafe cast. Consider adding type checking or using the toValue method for consistency.
@Override public URI copy(Object obj) { - // a URI is immutable - return (URI) obj; + if (obj instanceof URI) { + // a URI is immutable + return (URI) obj; + } + return toValue(obj); }
Line range hint
31-33: Consider making constructor privateSince this adapter appears to be singleton-managed (based on the AI summary mentioning registration in MetaschemaDataTypeProvider), the constructor should be private to prevent direct instantiation.
- UriAdapter() { + private UriAdapter() { super(URI.class); }
Line range hint
44-44: Remove unnecessary @SuppressWarningsThe
@SuppressWarnings("null")annotation appears unnecessary as there are no obvious null-related warnings in the parse method.- @SuppressWarnings("null")core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/AbstractIntegerAdapter.java (1)
42-46: Enhanced error handling looks good, consider adding input validationThe wrapping of
NumberFormatExceptioninIllegalArgumentExceptionis a good practice for method parameter validation. However, consider these improvements:@Override public BigInteger parse(String value) { + if (value == null) { + throw new IllegalArgumentException("Input value cannot be null"); + } try { return new BigInteger(value); } catch (NumberFormatException ex) { - throw new IllegalArgumentException(ex); + throw new IllegalArgumentException( + String.format("Invalid integer format: '%s'", value), ex); } }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDurationItem.java (2)
14-16: Consider enhancing interface documentationWhile the current documentation is clear, it would be beneficial to add more details about:
- The supported duration formats (e.g., ISO 8601)
- Examples of valid duration values
- The relationship with
IDayTimeDurationItemandIYearMonthDurationItem/** * A Metapath atomic item representing a duration data value. + * + * <p>This interface supports both day-time and year-month duration formats + * following the ISO 8601 standard. Examples of valid durations include: + * <ul> + * <li>P1Y2M (1 year, 2 months)</li> + * <li>P3DT4H5M (3 days, 4 hours, 5 minutes)</li> + * </ul> + * + * @see IDayTimeDurationItem + * @see IYearMonthDurationItem */
34-46: LGTM! Robust error handling implementationThe new implementation properly handles error cases and maintains error context. Good practices observed:
- Single evaluation of
asString()- Proper exception chaining
- Graceful fallback between duration types
One minor suggestion to improve error reporting:
- InvalidValueForCastFunctionException newEx = new InvalidValueForCastFunctionException(ex2); + InvalidValueForCastFunctionException newEx = new InvalidValueForCastFunctionException( + String.format("Value '%s' cannot be parsed as either a day-time or year-month duration", value), + ex2);core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/Base64BinaryItemImpl.java (3)
18-21: Consider enhancing the class documentation.While the current documentation is clear, it would be helpful to add:
- Information about the underlying ByteBuffer representation
- Any thread-safety considerations
- Usage examples
57-61: Consider enhancing the equals implementation.While the current implementation works, consider:
- Adding explicit null checking
- Refactoring to avoid the PMD suppression warning
@Override public boolean equals(Object obj) { + if (obj == null) { + return false; + } if (this == obj) { return true; } if (!(obj instanceof IBase64BinaryItem)) { return false; } return compareTo((IBase64BinaryItem) obj) == 0; }
76-79: Maintain consistency in equals implementation style.For consistency with the suggested outer class improvements, consider applying the same equals pattern to the MapKey class.
@Override public boolean equals(Object obj) { + if (obj == null) { + return false; + } if (this == obj) { return true; } if (!(obj instanceof MapKey)) { return false; } return getKey().equals(((MapKey) obj).getKey()); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DayTimeDurationItemImpl.java (3)
18-21: Consider enhancing class documentation.While the current documentation is clear, it could be more helpful by including:
- Example usage
- Details about duration format and constraints
Example enhancement:
/** * An implementation of a Metapath atomic item containing a duration data value * in days, hours, and seconds. + * + * <p>The duration is stored internally using Java's {@link Duration} class. + * + * <p>Example usage: + * <pre> + * Duration duration = Duration.ofDays(2).plusHours(3); + * DayTimeDurationItemImpl item = new DayTimeDurationItemImpl(duration); + * </pre> */
59-61: Consider adding explicit null check in equals().While instanceof handles null implicitly, an explicit null check would make the code more maintainable and clearer about its null handling behavior.
@SuppressWarnings("PMD.OnlyOneReturn") @Override public boolean equals(Object obj) { + if (obj == null) { + return false; + } return this == obj || obj instanceof IDayTimeDurationItem && compareTo((IDayTimeDurationItem) obj) == 0; }
76-79: Apply consistent null handling in MapKey.equals().For consistency with the outer class, consider adding an explicit null check here as well.
@Override public boolean equals(Object obj) { + if (obj == null) { + return false; + } return this == obj || obj instanceof MapKey && getKey().equals(((MapKey) obj).getKey()); }core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DecimalAdapter.java (1)
Line range hint
25-75: Excellent architectural improvement!The removal of casting-related methods from this adapter and moving them to atomic item implementations shows good separation of concerns. This makes the adapter more focused on its primary responsibility of handling decimal values.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/YearMonthDurationItemImpl.java (3)
19-22: Consider enhancing the class documentation.While the current documentation is clear, it would be beneficial to add more details about:
- The underlying
Periodclass usage- Any constraints or limitations on the duration values
- Examples of valid durations
/** * An implementation of a Metapath atomic item containing a duration data value * in years and months. + * + * This implementation uses Java's {@link Period} class to represent durations, + * supporting positive and negative values for years and months. + * + * @see java.time.Period */
60-62: Consider adding explicit null check in equals method.While instanceof handles null, explicit null checking improves code clarity and maintainability.
@SuppressWarnings("PMD.OnlyOneReturn") // readability @Override public boolean equals(Object obj) { + if (obj == null) { + return false; + } return this == obj || obj instanceof IYearMonthDurationItem && compareTo((IYearMonthDurationItem) obj) == 0; }
77-80: Add @OverRide annotations for interface methods.While the implementation is correct, adding @OverRide annotations improves code maintainability and helps catch errors during compilation if the interface changes.
@Override public boolean equals(Object obj) { + @Override return this == obj || obj instanceof MapKey && getKey().equals(((MapKey) obj).getKey()); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractStringItem.java (2)
Line range hint
24-27: Consider adding documentation for regex patterns.The static regex patterns would benefit from documentation explaining their purpose and the specific whitespace characters they handle.
+ /** Pattern to match one or more whitespace characters at the end of a string */ private static final Pattern TRIM_END = Pattern.compile(WHITESPACE_SEGMENT + "++$"); + /** Pattern to match one or more whitespace characters at the start of a string */ private static final Pattern TRIM_START = Pattern.compile("^" + WHITESPACE_SEGMENT + "+"); + /** Pattern to match two or more consecutive whitespace characters */ private static final Pattern TRIM_MIDDLE = Pattern.compile(WHITESPACE_SEGMENT + "{2,}");
Line range hint
71-79: Consider optimizing string normalization.The
normalizeSpacemethod could be optimized by:
- Combining multiple regex operations
- Using StringBuilder for better performance with multiple string operations
Here's a suggested optimization:
@Override public IStringItem normalizeSpace() { String value = asString(); - value = TRIM_START.matcher(value).replaceFirst(""); - value = TRIM_MIDDLE.matcher(value).replaceAll(" "); - value = TRIM_END.matcher(value).replaceFirst(""); - assert value != null; + value = value.strip().replaceAll("\\s+", " "); return IStringItem.valueOf(value); }This approach:
- Uses
strip()for trimming both ends (Java 11+)- Combines middle space normalization into a single regex
- Removes unnecessary null assertion (strip() never returns null)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/UuidItemImpl.java (3)
20-25: Consider enhancing class documentationWhile the current documentation is accurate, it could be more informative by including:
- The purpose of UUID items in the Metapath context
- The relationship with the parent class and interface
- Usage examples
Example enhancement:
/** * An implementation of a Metapath atomic item containing a UUID data value. + * + * This class provides UUID-specific functionality for Metapath operations, + * ensuring type-safe handling of UUID values within the Metaschema framework. + * It extends AbstractAnyAtomicItem for basic atomic item behavior and implements + * IUuidItem for UUID-specific operations. + * + * @see IUuidItem + * @see AbstractAnyAtomicItem */
Line range hint
69-73: Document the no-op behavior of normalizeSpaceConsider adding a method-level comment explaining why this is a no-op operation for UUID values.
+ /** + * Returns this instance unchanged since UUID string representations + * are already normalized. + * + * @return this instance + */ @Override public IStringItem normalizeSpace() { // noop return this; }
88-91: Consider caching the UUID string representationThe current implementation may repeatedly convert UUIDs to strings in equals/hashCode operations. Consider caching the string representation for better performance.
private final class MapKey implements IMapKey { + private final String cachedString; + + private MapKey() { + this.cachedString = getKey().asUuid().toString(); + } + @Override public int hashCode() { - return getKey().hashCode(); + return cachedString.hashCode(); } }core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/Base64Adapter.java (1)
55-55: Consider adding input validation before decoding.While ensuring non-null return is good, the method should validate the input string before attempting to decode it. Invalid base64 input will throw IllegalArgumentException from the decoder.
@Override public ByteBuffer parse(String value) { + if (value == null) { + throw new IllegalArgumentException("Input value cannot be null"); + } Base64.Decoder decoder = Base64.getDecoder(); byte[] result = decoder.decode(value); return ObjectUtils.notNull(ByteBuffer.wrap(result)); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IAnyAtomicItem.java (1)
17-19: Fix typo and enhance interface documentationThere's a typo in the interface documentation, and it could be more descriptive about what an "atomic item" represents.
/** - * The interface shares by all atomic items. + * The interface shared by all atomic items, representing indivisible data values + * that serve as the fundamental building blocks for complex data structures in the + * Metaschema framework. */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv6AddressItem.java (1)
21-40: LGTM! Well-implemented factory method with proper error handling.The implementation includes proper null checking, uses the type provider for parsing, and provides detailed error messages. Consider using String.format's positional arguments for better maintainability.
- String.format("The value '%s' is not a valid IPv6 address. %s", + String.format("The value '%1$s' is not a valid IPv6 address. %2$s",core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv4AddressItem.java (1)
33-38: Consider improving error handling construction.
- The
nullparameter in theInvalidTypeMetapathExceptionconstructor seems suspicious and should be reviewed.- String concatenation in error messages could be more efficient using a StringBuilder or String.format with fewer concatenations.
- throw new InvalidTypeMetapathException( - null, - String.format("The value '%s' is not a valid IPv4 address. %s", - value, - ex.getLocalizedMessage()), - ex); + throw new InvalidTypeMetapathException( + "IPv4 parsing", // or appropriate context + String.format("Invalid IPv4 address '%s': %s", value, ex.getMessage()), + ex);core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUriReferenceItem.java (3)
17-19: Consider enhancing interface documentationWhile the current documentation is clear, it could be more informative by:
- Explaining what constitutes a valid URI reference
- Mentioning RFC2396 compliance requirements
- Adding
@seetags to related classes/specifications/** - * A Metapath atomic item containing a URI reference data value. + * A Metapath atomic item containing a URI reference data value that complies with RFC2396. + * URI references can be absolute URIs, relative URIs, or same-document references. + * + * @see java.net.URI + * @see <a href="https://www.ietf.org/rfc/rfc2396.txt">RFC2396</a> */
21-42: LGTM! Well-implemented factory method with proper error handlingThe implementation is robust and follows best practices. Consider using String.format's positional arguments for better maintainability:
- String.format("The value '%s' is not a valid URI reference. %s", + String.format("The value '%1$s' is not a valid URI reference. %2$s",
68-77: Consider improving readability of cast logicWhile the implementation is functionally correct, the nested ternary operator could be refactored for better readability.
- try { - return item instanceof IUriReferenceItem - ? (IUriReferenceItem) item - : item instanceof IAnyUriItem - ? valueOf(((IAnyUriItem) item).asUri()) - : valueOf(item.asString()); - } catch (IllegalStateException | InvalidTypeMetapathException ex) { - // asString can throw IllegalStateException exception - throw new InvalidValueForCastFunctionException(ex); - } + try { + if (item instanceof IUriReferenceItem) { + return (IUriReferenceItem) item; + } + + if (item instanceof IAnyUriItem) { + return valueOf(((IAnyUriItem) item).asUri()); + } + + return valueOf(item.asString()); + } catch (IllegalStateException | InvalidTypeMetapathException ex) { + throw new InvalidValueForCastFunctionException(ex); + }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/BooleanItemImpl.java (2)
6-6: Good architectural change moving implementation to.implpackage!The relocation of implementation classes to a dedicated
.implpackage improves the separation between API and implementation, making the codebase more maintainable.Also applies to: 10-11
17-22: Consider enhancing class documentationWhile the current documentation is clear, it could be more comprehensive by including:
- The relationship with IBooleanItem interface
- The purpose of extending AbstractAtomicItemBase
- Usage examples or common scenarios
Example enhancement:
/** * An implementation of a Metapath atomic item with a boolean value. + * This class provides a concrete implementation of {@link IBooleanItem} + * by extending {@link AbstractAtomicItemBase<Boolean>} to leverage common + * atomic item behavior while maintaining type safety. + * + * @see IBooleanItem + * @see AbstractAtomicItemBase */core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateTimeAdapter.java (1)
56-73: Consider extracting parsing logic into separate methods.While the error handling is robust, the method could be more readable by extracting the parsing logic into separate methods for each format.
Consider refactoring like this:
public DateTime parse(String value) { - DateTime retval; - try { - retval = new DateTime( - ObjectUtils.notNull(ZonedDateTime.from(DateFormats.DATE_TIME_WITH_TZ.parse(value))), - true); - } catch (DateTimeParseException ex) { - try { - LocalDateTime dateTime = LocalDateTime.from(DateFormats.DATE_TIME_WITHOUT_TZ.parse(value)); - retval = new DateTime( - ObjectUtils.notNull(ZonedDateTime.of(dateTime, ZoneOffset.UTC)), - false); - } catch (DateTimeParseException ex2) { - IllegalArgumentException newEx = new IllegalArgumentException(ex2.getLocalizedMessage(), ex2); - newEx.addSuppressed(ex); - throw newEx; - } - } - return retval; + try { + return parseWithTimeZone(value); + } catch (DateTimeParseException ex) { + try { + return parseWithoutTimeZone(value); + } catch (DateTimeParseException ex2) { + throw createParseException(ex, ex2); + } + } + } + + private DateTime parseWithTimeZone(String value) throws DateTimeParseException { + return new DateTime( + ObjectUtils.notNull(ZonedDateTime.from(DateFormats.DATE_TIME_WITH_TZ.parse(value))), + true); + } + + private DateTime parseWithoutTimeZone(String value) throws DateTimeParseException { + LocalDateTime dateTime = LocalDateTime.from(DateFormats.DATE_TIME_WITHOUT_TZ.parse(value)); + return new DateTime( + ObjectUtils.notNull(ZonedDateTime.of(dateTime, ZoneOffset.UTC)), + false); + } + + private IllegalArgumentException createParseException(DateTimeParseException ex1, DateTimeParseException ex2) { + IllegalArgumentException newEx = new IllegalArgumentException(ex2.getLocalizedMessage(), ex2); + newEx.addSuppressed(ex1); + return newEx;core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUuidItem.java (2)
17-42: LGTM with minor documentation suggestion.The implementation is robust with proper error handling and null-safety. However, there's a small documentation issue in the Javadoc.
- * Construct a new URI item using the provided string {@code value}. + * Construct a new UUID item using the provided string {@code value}.
79-86: Consider more specific error handling.While the current implementation catches both IllegalStateException and InvalidTypeMetapathException, it might be beneficial to provide more specific error messages for each case.
try { return item instanceof IUuidItem ? (IUuidItem) item : valueOf(item.asString()); - } catch (IllegalStateException | InvalidTypeMetapathException ex) { - // asString can throw IllegalStateException exception - throw new InvalidValueForCastFunctionException(ex); + } catch (IllegalStateException ex) { + throw new InvalidValueForCastFunctionException( + String.format("Failed to convert value to string: %s", ex.getMessage()), ex); + } catch (InvalidTypeMetapathException ex) { + throw new InvalidValueForCastFunctionException( + String.format("Failed to parse UUID: %s", ex.getMessage()), ex); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBase64BinaryItem.java (2)
34-43: Consider extracting the magic number to a constant.The implementation looks good with proper error handling and informative messages. However, the value 200 used for truncation could be extracted to a named constant for better maintainability.
+ private static final int MAX_ERROR_PREVIEW_LENGTH = 200; @NonNull static IBase64BinaryItem valueOf(@NonNull String value) { try { return valueOf(MetaschemaDataTypeProvider.BASE64.parse(value)); } catch (IllegalArgumentException ex) { throw new InvalidTypeMetapathException( null, String.format("The value starting with '%s' is not a valid Base64 character sequence. %s", - value.substring(0, Math.min(value.length(), 200)), + value.substring(0, Math.min(value.length(), MAX_ERROR_PREVIEW_LENGTH)), ex.getLocalizedMessage()), ex); } }
71-78: Consider adding more context to the error message.The implementation correctly handles type casting and exceptions. However, the error message could be more descriptive when wrapping the original exception.
try { return item instanceof IBase64BinaryItem ? (IBase64BinaryItem) item : valueOf(item.asString()); } catch (IllegalStateException | InvalidTypeMetapathException ex) { // asString can throw IllegalStateException exception - throw new InvalidValueForCastFunctionException(ex); + throw new InvalidValueForCastFunctionException( + String.format("Failed to cast %s to Base64Binary: %s", + item.getClass().getSimpleName(), + ex.getMessage()), + ex); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDayTimeDurationItem.java (1)
38-43: Consider using String.format constants for error messagesThe error handling implementation is good, with proper exception wrapping and detailed messaging. However, consider extracting the error message format string to a constant for better maintainability.
+ private static final String ERR_INVALID_DURATION = "The value '%s' is not a valid day/time duration. %s"; - String.format("The value '%s' is not a valid day/time duration. %s", + String.format(ERR_INVALID_DURATION, value, ex.getLocalizedMessage())core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IPositiveIntegerItem.java (1)
40-47: Consider documenting the null parameter usage.The error handling improvements are good, providing more detailed feedback. However, the
nullparameter passed toInvalidTypeMetapathExceptionshould be documented to explain its purpose.throw new InvalidTypeMetapathException( - null, + null, // null because there's no associated expression context String.format("The value '%s' is not a valid positive integer. %s",core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IAnyUriItem.java (1)
33-42: Consider providing context information in the exception constructor.The implementation looks good overall, but the
nullparameter in theInvalidTypeMetapathExceptionconstructor could be enhanced by providing context information about where the error occurred.- null, + "Error in IAnyUriItem.valueOf()",core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INumericItem.java (2)
21-23: Enhance interface documentation.While the current documentation is accurate, consider expanding it to include:
- Types of numeric values supported (e.g., integers, decimals)
- Relationship with other atomic item types
- Example usage scenarios
/** * A Metapath atomic item containing a numeric data value. + * + * <p>This interface represents numeric values in the Metapath type system, + * supporting both integer ({@link BigInteger}) and decimal ({@link BigDecimal}) + * values. It provides operations for type conversion, arithmetic operations, + * and rounding.</p> + * + * <p>Implementation classes include {@link IIntegerItem} and {@link IDecimalItem}.</p> + * + * @see IIntegerItem + * @see IDecimalItem */
Line range hint
114-182: Consider refactoring complex rounding logic.The round method has high cognitive complexity (suppressed PMD warning) and handles multiple cases. Consider breaking it down into smaller, focused methods for better maintainability.
+ private INumericItem roundWithPositivePrecision(int precision) { + if (this instanceof IIntegerItem) { + return this; + } + BigDecimal value = asDecimal(); + BigDecimal rounded = value.signum() == -1 + ? value.round(new MathContext(precision + value.precision() - value.scale(), RoundingMode.HALF_DOWN)) + : value.round(new MathContext(precision + value.precision() - value.scale(), RoundingMode.HALF_UP)); + return castAsType(IDecimalItem.valueOf(ObjectUtils.notNull(rounded))); + } + private INumericItem roundWithNegativePrecision(int precision) { + BigInteger value = asInteger(); + BigInteger divisor = BigInteger.TEN.pow(0 - precision); + if (divisor.compareTo(value.abs()) > 0) { + return IIntegerItem.valueOf(BigInteger.ZERO); + } + BigInteger remainder = value.mod(divisor); + BigInteger lessRemainder = value.subtract(remainder); + BigInteger halfDivisor = divisor.divide(BigInteger.TWO); + BigInteger result = remainder.compareTo(halfDivisor) >= 0 + ? lessRemainder.add(divisor) + : lessRemainder; + return IIntegerItem.valueOf(ObjectUtils.notNull(result)); + } @SuppressWarnings("PMD.CognitiveComplexity") // complexity moved to helper methods @NonNull default INumericItem round(@NonNull IIntegerItem precisionItem) { int precision; try { precision = FunctionUtils.asInteger(precisionItem); } catch (ArithmeticException ex) { throw new ArithmeticFunctionException( ArithmeticFunctionException.OVERFLOW_UNDERFLOW_ERROR, "Numeric operation overflow/underflow.", ex); } return precision >= 0 ? roundWithPositivePrecision(precision) : roundWithNegativePrecision(precision); }core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/IMarkupString.java (1)
Line range hint
37-180: Interface simplification aligns with PR objectives.The removal of the
writeHtmlmethod (as noted in the AI summary) while retainingwriteXHtmlmethods is a good refactoring decision that:
- Reduces interface complexity
- Eliminates potential redundancy
- Maintains clear separation of concerns
The remaining methods provide a comprehensive and well-organized API for markup string manipulation.
Consider documenting in the interface's Javadoc or README why
writeXHtmlis preferred overwriteHtmlto help future maintainers understand this architectural decision.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnMinMax.java (2)
46-55: Add Javadoc to document the purpose of PRIMITIVE_ITEM_TYPES.The constant is well-implemented using an immutable set and proper null-safety annotations. However, adding Javadoc would improve maintainability by documenting:
- The purpose of this set
- Why these specific types are included
- How it differs from IAnyAtomicItem.PRIMITIVE_ITEM_TYPES (mentioned in the AI summary)
+ /** + * Defines the set of primitive atomic item types supported by the min/max functions. + * This set is used for type validation and normalization during comparison operations. + */ @NonNull private static final Set<Class<? extends IAnyAtomicItem>> PRIMITIVE_ITEM_TYPES = ObjectUtils.notNull(Set.of(
172-172: Consider refactoring the normalize method for better maintainability.While using
PRIMITIVE_ITEM_TYPESimproves the code, thenormalizemethod has multiple responsibilities. Consider breaking it down into smaller, focused methods:private static Stream<? extends IAnyAtomicItem> normalize( @NonNull List<? extends IAnyAtomicItem> items) { if (items.isEmpty()) { return Stream.empty(); } if (items.size() == 1) { return Stream.of(items.get(0)); } - List<? extends IAnyAtomicItem> resultingItems = ObjectUtils.notNull(items.stream() - .map(item -> item instanceof IUntypedAtomicItem ? IDecimalItem.cast(item) : item) - .collect(Collectors.toList())); - - Map<Class<? extends IAnyAtomicItem>, Integer> counts = FunctionUtils.countTypes( - PRIMITIVE_ITEM_TYPES, - resultingItems); + List<? extends IAnyAtomicItem> resultingItems = convertUntypedItems(items); + Map<Class<? extends IAnyAtomicItem>, Integer> counts = countItemTypes(resultingItems); + return createNormalizedStream(resultingItems, counts); + } + + private static List<? extends IAnyAtomicItem> convertUntypedItems( + List<? extends IAnyAtomicItem> items) { + return ObjectUtils.notNull(items.stream() + .map(item -> item instanceof IUntypedAtomicItem ? IDecimalItem.cast(item) : item) + .collect(Collectors.toList())); + } + + private static Map<Class<? extends IAnyAtomicItem>, Integer> countItemTypes( + List<? extends IAnyAtomicItem> items) { + return FunctionUtils.countTypes(PRIMITIVE_ITEM_TYPES, items); + } + + private static Stream<? extends IAnyAtomicItem> createNormalizedStream( + List<? extends IAnyAtomicItem> items, + Map<Class<? extends IAnyAtomicItem>, Integer> counts) { Stream<? extends IAnyAtomicItem> stream = null; if (counts.size() == 1) { - stream = resultingItems.stream(); + stream = items.stream(); } else if (counts.size() > 1) { - int size = resultingItems.size(); + int size = items.size(); if (counts.getOrDefault(IStringItem.class, 0) + counts.getOrDefault(IAnyUriItem.class, 0) == size) { - stream = resultingItems.stream() + stream = items.stream() .map(IAnyAtomicItem::asStringItem); } else if (counts.getOrDefault(IDecimalItem.class, 0) == size) { - stream = resultingItems.stream() + stream = items.stream() .map(item -> (IDecimalItem) item); } } if (stream == null) { throw new InvalidArgumentFunctionException( InvalidArgumentFunctionException.INVALID_ARGUMENT_TYPE, String.format("Values must all be of a single atomic type. Their types are '%s'.", - FunctionUtils.getTypes(resultingItems).stream() + FunctionUtils.getTypes(items).stream() .map(Class::getName) .collect(Collectors.joining(",")))); } return stream;This refactoring:
- Improves readability by giving each operation a clear name
- Makes the code easier to test and maintain
- Follows the Single Responsibility Principle
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/AbstractDataTypeAdapter.java (1)
199-241: Document the new casting strategy in class-level Javadoc.Since this is a significant architectural change, the class-level documentation should be updated to explain:
- Why casting is no longer handled here
- Where casting operations now reside
- How subclasses should handle type conversion
Consider adding the following to the class-level Javadoc:
/** * Provides a basic Java type adapter implementation. This implementation should * be the parent class of all Java type adapter implementations. + * + * <p>Note: As of version X.Y.Z, casting operations have been moved to atomic item + * implementations to improve separation between type adapters and Metapath items. + * Type conversion should now be handled through the respective atomic item classes.</p> * * @param <TYPE> * the raw Java type this adapter supportscore/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/MetaschemaDataTypeProvider.java (2)
16-18: Consider reducing coupling through lazy initialization.The suppressed PMD warning about coupling between objects suggests high complexity. Consider implementing lazy initialization for the adapter instances to reduce coupling and improve startup performance.
Example pattern:
private static volatile Base64Adapter base64Instance; @NonNull public static Base64Adapter BASE64() { Base64Adapter result = base64Instance; if (result == null) { synchronized (MetaschemaDataTypeProvider.class) { result = base64Instance; if (result == null) { base64Instance = result = new Base64Adapter(); } } } return result; }
Line range hint
182-213: Document why NCNAME is not registered.The deprecated NCNAME adapter is declared as a field but not registered in the constructor. Please add a comment explaining this omission or register it if needed.
registerDatatype(IP_V4_ADDRESS); registerDatatype(IP_V6_ADDRESS); +// NCNAME is deprecated and intentionally not registeredmetaschema-cli/src/test/java/gov/nist/secauto/metaschema/cli/CLITest.java (1)
218-221: Consider using text blocks for improved readability.While splitting the long error messages with string concatenation improves readability, consider using Java text blocks (""") for a more maintainable approach to handle multi-line strings.
Here's how you could refactor these lines:
- "matches-custom-regex-letters-only: No default message, custom error message for" + - " matches-custom-regex-letters-only constraint.", - "cardinality-default-two-minimum: The cardinality '1' is below the required minimum '2' for items" + - " matching", + """ + matches-custom-regex-letters-only: No default message, custom error message for \ + matches-custom-regex-letters-only constraint.""", + """ + cardinality-default-two-minimum: The cardinality '1' is below the required minimum '2' for items \ + matching""",This approach:
- Eliminates the need for string concatenation
- Maintains the same formatting in the source code as in the output
- Makes it easier to edit the messages in the future
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/ComparisonFunctions.java (1)
Line range hint
144-147: Fix duplicate condition in if-else chainThere's a duplicate condition for
IDayTimeDurationItemwhich makes the second condition unreachable:} else if (item instanceof IDayTimeDurationItem) { retval = IDayTimeDurationItem.cast(other); } else if (item instanceof IDayTimeDurationItem) { retval = IYearMonthDurationItem.cast(other);Apply this fix:
} else if (item instanceof IDayTimeDurationItem) { retval = IDayTimeDurationItem.cast(other); - } else if (item instanceof IDayTimeDurationItem) { + } else if (item instanceof IYearMonthDurationItem) { retval = IYearMonthDurationItem.cast(other);core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/IntegerAdapter.java (1)
19-23: Ensure JavaDoc link is correctly formattedThe JavaDoc comment contains a hyperlink with a line break within the
hrefattribute. This may cause issues when generating documentation. It's recommended to avoid line breaks within thehrefattribute.Apply this diff to fix the JavaDoc comment:
/** - * Support for the Metaschema <a href= - * "https://pages.nist.gov/metaschema/specification/datatypes/#integer">integer</a> - * data type. + * Support for the Metaschema <a href="https://pages.nist.gov/metaschema/specification/datatypes/#integer">integer</a> + * data type. */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPAddressItem.java (1)
53-55: HandleIllegalStateExceptionto provide more informative error messagesThe current
catchblock forIllegalStateExceptiondoes nothing, which may result invaluebeingnulland less informative error messages. Consider settingvalueto a placeholder or including the exception message to enhance debugging.Apply this diff to improve exception handling:
} catch (IllegalStateException ex) { - // do nothing + value = "<unavailable>"; + // Optionally, log the exception or include ex.getMessage() if appropriate }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IHostnameItem.java (1)
16-17: Typographical error in Javadoc commentIn the Javadoc comment, "an host name value" should be corrected to "a host name value".
Apply this diff to fix the typographical error:
- * a string representing an host name value + * a string representing a host name valuecore/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INcNameItem.java (2)
15-17: Clarify documentation terminologyIn the class comment, consider adding the abbreviation "(NCName)" after "non-colonized name" for clarity:
/** * A Metapath atomic item containing a non-colonized name (NCName) data value. */This makes it immediately clear to readers familiar with XML terminology.
54-61: Review exception handling for clarity and consistencyIn the
castmethod, you're catchingIllegalStateExceptionandInvalidTypeMetapathException, then wrapping them intoInvalidValueForCastFunctionException. Consider if wrapping the exceptions adds necessary value or if it obscures the original exception context. Potential improvements:
- If the
InvalidTypeMetapathExceptionalready provides sufficient information, you might let it propagate without wrapping.- If wrapping is necessary, ensure that the
InvalidValueForCastFunctionExceptionincludes all relevant details from the original exceptions.This helps maintain clear exception hierarchies and improves debuggability.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDateTimeItem.java (2)
19-21: Improve Javadoc comment for clarityConsider rephrasing the class description for better clarity and grammatical correctness. For example:
- * A Metapath atomic item containing a date/time data value. + * An atomic Metapath item representing a date/time value.
35-39: Enhance exception handling by providing context and message consistencyPassing
nullas the first argument toInvalidTypeMetapathExceptionmay omit valuable context for debugging. If possible, provide the relevant location or context information. Additionally, usingex.getLocalizedMessage()may result in inconsistent messages. Consider usingex.getMessage()for a more standard exception message.core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapter.java (1)
30-34: Nitpick: Use JavaDoc@seeannotation instead of raw HTML tagsUsing raw HTML tags in JavaDoc comments can lead to rendering issues in some documentation tools. Consider using the
@seeannotation for better compatibility and readability.Apply this diff to update the JavaDoc:
/** - * Support for the Metaschema <a href= - * "https://pages.nist.gov/metaschema/specification/datatypes/#date">date</a> - * data type. + * Support for the Metaschema date data type. + * @see <a href="https://pages.nist.gov/metaschema/specification/datatypes/#date">Metaschema Date Type</a> */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java (1)
26-26: Remove unnecessary use ofObjectUtils.notNullwith constantsThe constants
BigInteger.ONEandBigInteger.ZEROare guaranteed to be non-null. Wrapping them withObjectUtils.notNullis redundant and can be omitted for cleaner code.Apply this diff to simplify the initialization:
- INonNegativeIntegerItem ONE = valueOf(ObjectUtils.notNull(BigInteger.ONE)); + INonNegativeIntegerItem ONE = valueOf(BigInteger.ONE); ... - INonNegativeIntegerItem ZERO = valueOf(ObjectUtils.notNull(BigInteger.ZERO)); + INonNegativeIntegerItem ZERO = valueOf(BigInteger.ZERO);Also applies to: 31-31
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IYearMonthDurationItem.java (2)
18-20: Documentation enhanced for better clarityThe Javadoc comments for the class provide a clear description of its purpose, improving code readability and maintainability.
39-43: Improved exception handling with detailed messagesThe code now throws an
InvalidTypeMetapathExceptionwith a detailed error message that includes the invalid value and the original exception's message. This enhances error reporting and will aid in debugging.Consider simplifying the exception message construction for readability:
throw new InvalidTypeMetapathException( null, - String.format("The value '%s' is not a valid year/month duration. %s", - value, - ex.getLocalizedMessage()), + String.format("Invalid year/month duration value '%s': %s", + value, + ex.getMessage()), ex);core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBooleanItem.java (2)
49-55: Simplify Exception Message for ClarityThe current exception message could be more concise. Consider revising it to improve readability and provide clearer information to the user.
Apply this diff to streamline the exception message:
- throw new InvalidTypeMetapathException( - null, - String.format("The value '%s' is not a valid boolean value. %s", - value, - ex.getLocalizedMessage()), - ex); + throw new InvalidTypeMetapathException( + null, + String.format("Invalid boolean value: '%s'. %s", + value, + ex.getMessage()), + ex);This change removes unnecessary wording and focuses on the essential details of the error.
105-108: Unnecessary Override ofcastAsTypeMethodThe default implementation of
castAsType(IAnyAtomicItem item)simply delegates tocast(item). Since this behavior might already be provided by the interface hierarchy, consider removing this override if it's redundant.If the override is required for specific interface compliance or clarity, it can remain as is.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIntegerItem.java (1)
25-25: Remove redundant null checks onBigIntegerconstants
BigInteger.ONEandBigInteger.ZEROare constants that are guaranteed to be non-null. UsingObjectUtils.notNullon these constants is unnecessary and can be removed to simplify the code.Apply this diff to simplify the code:
- IIntegerItem ONE = valueOf(ObjectUtils.notNull(BigInteger.ONE)); + IIntegerItem ONE = valueOf(BigInteger.ONE); - IIntegerItem ZERO = valueOf(ObjectUtils.notNull(BigInteger.ZERO)); + IIntegerItem ZERO = valueOf(BigInteger.ZERO); - IIntegerItem NEGATIVE_ONE = valueOf(ObjectUtils.notNull(BigInteger.ONE.negate())); + IIntegerItem NEGATIVE_ONE = valueOf(BigInteger.ONE.negate()); - return valueOf(ObjectUtils.notNull(value ? BigInteger.ONE : BigInteger.ZERO)); + return value ? ONE : ZERO;Also applies to: 30-30, 35-35, 96-96
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (103)
core/src/main/java-templates/gov/nist/secauto/metaschema/core/MetaschemaJavaVersion.java(1 hunks)core/src/main/java-templates/gov/nist/secauto/metaschema/core/model/MetaschemaVersion.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/AbstractCustomJavaDataTypeAdapter.java(0 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/AbstractDataTypeAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/IDataTypeAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/AbstractIntegerAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/Base64Adapter.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/BooleanAdapter.java(0 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapter.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateTimeAdapter.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateTimeWithTZAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateWithTZAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DayTimeAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DecimalAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/EmailAddressAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/HostnameAdapter.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/IPv4AddressAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/IPv6AddressAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/IntegerAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/MetaschemaDataTypeProvider.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/NcNameAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/NonNegativeIntegerAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/PositiveIntegerAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/StringAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/TokenAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/UriAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/UriReferenceAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/UuidAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/YearMonthAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/AbstractMarkupAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/AbstractMarkupString.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/IMarkupString.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupDataTypeProvider.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupLine.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupMultiline.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/ComparisonFunctions.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/FunctionUtils.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnCeiling.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnMinMax.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/AbstractAnyAtomicItem.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/AbstractAtomicItemBase.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IAnyAtomicItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IAnyUriItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBase64BinaryItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBooleanItem.java(5 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDateItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDateTimeItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDayTimeDurationItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDecimalItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDurationItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IEmailAddressItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IHostnameItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPAddressItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv4AddressItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv6AddressItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIntegerItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IMarkupItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INcNameItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java(4 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INumericItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IPositiveIntegerItem.java(4 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IStringItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/ITemporalItem.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/ITokenItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUntypedAtomicItem.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUriReferenceItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUuidItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IYearMonthDurationItem.java(4 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractDateItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractDateTimeItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractDecimalItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractIntegerItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractStringItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractTemporalItem.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractUriItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AnyUriItemImpl.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/Base64BinaryItemImpl.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/BooleanItemImpl.java(5 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateTimeWithTimeZoneItemImpl.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateTimeWithoutTimeZoneItemImpl.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateWithTimeZoneItemImpl.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateWithoutTimeZoneItemImpl.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DayTimeDurationItemImpl.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DecimalItemImpl.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/EmailAddressItemImpl.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/HostnameItemImpl.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/IPv4AddressItemImpl.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/IPv6AddressItemImpl.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/IntegerItemImpl.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/MarkupLineItemImpl.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/MarkupMultiLineItemImpl.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/NcNameItemImpl.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/NonNegativeIntegerItemImpl.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/PositiveIntegerItemImpl.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/StringItemImpl.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/TokenItemImpl.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/UriReferenceItemImpl.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/UuidItemImpl.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/YearMonthDurationItemImpl.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/model/validation/XmlSchemaContentValidator.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnCeilingTest.java(2 hunks)metaschema-cli/src/test/java/gov/nist/secauto/metaschema/cli/CLITest.java(1 hunks)schemagen/src/test/java/gov/nist/secauto/metaschema/schemagen/AbstractSchemaGeneratorTestSuite.java(1 hunks)
💤 Files with no reviewable changes (2)
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/AbstractCustomJavaDataTypeAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/BooleanAdapter.java
✅ Files skipped from review due to trivial changes (21)
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateTimeWithTZAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateWithTZAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DayTimeAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/EmailAddressAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/HostnameAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/IPv4AddressAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/IPv6AddressAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/NcNameAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/NonNegativeIntegerAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/PositiveIntegerAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/StringAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/TokenAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/UriReferenceAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/YearMonthAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/AbstractMarkupAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/AbstractMarkupString.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupLine.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupMultiline.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnCeiling.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/ITemporalItem.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUntypedAtomicItem.java
🔇 Additional comments (171)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/StringItemImpl.java (2)
6-11: LGTM! Clean package structure and imports.
The move to an 'impl' package follows good separation of concerns, keeping implementation details separate from the interface definitions.
Line range hint 16-34: Implementation looks good. Verify parent class.
The implementation is clean and follows good practices with proper null-safety and documentation.
Let's verify the parent class and interface implementation:
✅ Verification successful
Parent class and interface verified successfully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and structure of parent class and interface
# Check for AbstractStringItem
ast-grep --pattern 'class AbstractStringItem {
$$$
}'
# Check for IStringItem interface
ast-grep --pattern 'interface IStringItem {
$$$
}'
Length of output: 112
Script:
#!/bin/bash
# Description: Search for subclasses of AbstractStringItem and implementations of IStringItem
# Search for any class extending AbstractStringItem
rg 'class\s+\w+\s+extends\s+AbstractStringItem' --files-with-matches
# Search for any interface implementing IStringItem
rg 'interface\s+\w+\s+extends\s+IStringItem' --files-with-matches
Length of output: 593
Script:
#!/bin/bash
# Description: Broad search for AbstractStringItem classes and IStringItem interfaces with naming variations
# Search for any abstract class containing 'StringItem'
rg 'abstract\s+class\s+\w*StringItem\w*' --files-with-matches
# Search for any interface containing 'StringItem'
rg 'interface\s+\w*StringItem\w*' --files-with-matches
Length of output: 312
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AnyUriItemImpl.java (3)
Line range hint 6-13: LGTM! Package organization follows best practices.
The move to the .impl package properly separates implementation from interface, improving code organization.
21-29: LGTM! Constructor is well-implemented and documented.
The constructor follows best practices with proper null-checking annotation and clear documentation.
Line range hint 18-34: Verify consistent usage after package relocation.
Since this class has been moved to the .impl package, we should verify that all references have been updated accordingly.
✅ Verification successful
Verification Successful: All references updated correctly.
- No remaining references to the old package path were found.
- All instantiations of
AnyUriItemImpluse the updated import from the.implpackage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old package path
rg "gov\.nist\.secauto\.metaschema\.core\.metapath\.item\.atomic\.AnyUriItemImpl"
# Search for current usage patterns to ensure consistent implementation
rg -A 2 "new AnyUriItemImpl\("
Length of output: 490
Script:
#!/bin/bash
# Check the import statement for AnyUriItemImpl in IAnyUriItem.java
rg 'import gov\.nist\.secauto\.metaschema\.core\.metapath\.item\.atomic\.impl\.AnyUriItemImpl;' core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IAnyUriItem.java
Length of output: 269
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/AbstractAtomicItemBase.java (5)
1-7: LGTM: Proper license and copyright headers
The file includes appropriate SPDX headers for copyright and licensing information.
12-18: LGTM: Well-documented class with clear purpose
The Javadoc clearly explains the class's purpose and documents the generic type parameter.
24-32: LGTM: Consistent string representation
The implementation properly delegates string conversion to the type adapter and maintains consistency between asString() and toString().
1-33: Verify impact of casting removal
The AI summary indicates that casting operations were removed from data type adapters. Let's verify that all atomic item implementations properly handle type conversion without relying on removed casting methods.
18-23: Consider strengthening the contract for getValue()
While getJavaTypeAdapter() is properly annotated with @NonNull, the inherited getValue() method from IAnyAtomicItem might benefit from explicit nullability annotations in the interface.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/IntegerItemImpl.java (3)
Line range hint 6-13: LGTM! Clean package organization and imports.
The move to an .impl package aligns with best practices for separating interface from implementation. Imports are minimal and well-organized.
Line range hint 30-33: LGTM! Proper type adapter implementation.
The getJavaTypeAdapter implementation correctly returns the singleton instance from MetaschemaDataTypeProvider.
21-28: Verify constructor visibility change impact.
The constructor visibility change from protected to public represents an API change that could affect extensibility. Let's verify there are no unintended consequences.
✅ Verification successful
Constructor visibility change impact verified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing subclasses that might be affected by the visibility change
# Search for classes extending IntegerItemImpl
ast-grep --pattern 'class $_ extends IntegerItemImpl'
# Search for protected constructor usage
rg -l "new IntegerItemImpl\(" --type java
Length of output: 190
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/AbstractAnyAtomicItem.java (2)
13-14: LGTM! Clear and accurate documentation.
The updated class documentation clearly describes the purpose and responsibility of this abstract class.
19-20: Verify subclass compatibility with new inheritance structure.
The inheritance change to AbstractAtomicItemBase<TYPE> aligns well with the refactoring goals. However, we should verify that all subclasses properly implement the required methods from the new base class.
✅ Verification successful
Inheritance change approved; no subclasses extend AbstractAnyAtomicItem.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all subclasses of AbstractAnyAtomicItem and verify they implement required methods
# Find all Java files that might extend AbstractAnyAtomicItem
echo "Searching for subclasses of AbstractAnyAtomicItem..."
ast-grep --pattern 'class $_ extends AbstractAnyAtomicItem<$_> {
$$$
}'
# Find potential missing implementations of getJavaTypeAdapter
echo "Checking for required method implementations..."
ast-grep --pattern 'class $_ extends AbstractAnyAtomicItem<$_> {
$$$
getJavaTypeAdapter() {
$$$
}
$$$
}'
Length of output: 414
Script:
#!/bin/bash
# Description: List all subclasses of AbstractAnyAtomicItem and check for getJavaTypeAdapter() implementation
echo "Listing all subclasses of AbstractAnyAtomicItem..."
rg 'class\s+\w+\s+extends\s+AbstractAnyAtomicItem<\w+>' --java -l
echo "Verifying implementation of getJavaTypeAdapter() in each subclass..."
rg 'class\s+\w+\s+extends\s+AbstractAnyAtomicItem<\w+>' --java -l | while read -r file; do
echo "Checking $file for getJavaTypeAdapter() implementation..."
rg 'getJavaTypeAdapter\s*\(' "$file" && echo "Implemented in $file" || echo "Missing in $file"
done
Length of output: 1226
Script:
#!/bin/bash
# Description: List all subclasses of AbstractAnyAtomicItem and check for getJavaTypeAdapter() implementation
echo "Listing all subclasses of AbstractAnyAtomicItem..."
rg 'class\s+\w+\s+extends\s+AbstractAnyAtomicItem<\w+>' --type java -l
echo "Verifying implementation of getJavaTypeAdapter() in each subclass..."
rg 'class\s+\w+\s+extends\s+AbstractAnyAtomicItem<\w+>' --type java -l | while read -r file; do
echo "Checking $file for getJavaTypeAdapter() implementation..."
rg 'getJavaTypeAdapter\s*\(' "$file" && echo "Implemented in $file" || echo "Missing in $file"
done
Length of output: 418
Script:
#!/bin/bash
# Description: List all subclasses of AbstractAnyAtomicItem and check for getJavaTypeAdapter() implementation
echo "Listing all subclasses of AbstractAnyAtomicItem..."
rg 'class\s+\w+\s+extends\s+AbstractAnyAtomicItem(?:<[^>]+>)?' --type java -l
echo "Verifying implementation of getJavaTypeAdapter() in each subclass..."
rg 'class\s+\w+\s+extends\s+AbstractAnyAtomicItem(?:<[^>]+>)?' --type java -l | while read -r file; do
echo "Checking $file for getJavaTypeAdapter() implementation..."
rg 'getJavaTypeAdapter\s*\(' "$file" && echo "Implemented in $file" || echo "Missing in $file"
done
Length of output: 432
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/TokenItemImpl.java (3)
6-11: LGTM! Good separation of interface and implementation
The move to an 'impl' subpackage follows best practices by clearly separating the implementation from the interface definition.
18-20: LGTM! Clean class structure
The class hierarchy is well-designed, properly extending AbstractStringItem while implementing the specific ITokenItem interface.
22-30: Verify potential exceptions from super constructor
The constructor documentation looks good, but we should verify if the super constructor can throw any exceptions that need to be documented.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/HostnameItemImpl.java (4)
6-12: LGTM! Package structure follows best practices
The move to the impl package properly separates implementation from interface, following the Interface Segregation Principle.
17-19: LGTM! Class structure is well-designed
The class follows SOLID principles with clear inheritance hierarchy and interface implementation.
Line range hint 31-34: LGTM! Method implementation is correct
The method properly returns the shared HostnameAdapter instance from MetaschemaDataTypeProvider.
21-29: Verify hostname format validation
While null checking is implemented, consider adding runtime validation of the hostname format to ensure data integrity.
Let's check if validation is handled elsewhere in the codebase:
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/EmailAddressItemImpl.java (3)
6-12: LGTM! Good package organization.
The move to the 'impl' package properly separates implementation from interface, following good architectural practices.
18-20: LGTM! Well-structured class hierarchy.
The class properly extends AbstractStringItem and implements IEmailAddressItem, following good OOP principles.
Line range hint 32-35: LGTM! Efficient adapter retrieval.
The method correctly returns the singleton instance from MetaschemaDataTypeProvider.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/NcNameItemImpl.java (1)
19-22: LGTM! Clean class structure
The class hierarchy and interface implementation are well-organized, following good object-oriented design principles.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/UriReferenceItemImpl.java (4)
6-14: LGTM! Good package organization.
The move to the impl package properly separates implementation from interface, following good software design practices.
20-22: LGTM! Clean class hierarchy.
The class structure properly extends AbstractUriItem while implementing the specific IUriReferenceItem interface, following good inheritance practices.
24-32: LGTM! Well-documented and null-safe constructor.
The constructor is properly documented and uses @nonnull annotation to ensure type safety.
Line range hint 20-37: Verify consistent implementation across atomic items.
Let's ensure this implementation follows the same pattern as other atomic items in the codebase.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateWithTimeZoneItemImpl.java (2)
Line range hint 6-13: LGTM! Clean package structure and imports
The package reorganization to the 'impl' subpackage improves code organization, and the imports are well-organized and minimal.
19-30: LGTM! Well-structured class and constructor
The class design is clean with proper inheritance, and the constructor is well-documented with appropriate null-safety checks.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/PositiveIntegerItemImpl.java (3)
6-6: LGTM: Package restructuring improves separation of concerns
The move to an .impl package and addition of the interface import follows good practices by separating interface from implementation.
Also applies to: 10-10
34-37: LGTM: Proper use of singleton adapter
The implementation correctly returns the singleton instance of the PositiveIntegerAdapter.
24-30: Verify if public constructor is intended
The constructor visibility has been changed to public. Please confirm if direct instantiation of this implementation class is intended, as it might be preferable to use a factory method or builder pattern.
✅ Verification successful
Constructor Visibility Confirmed as Intended
The public constructor is actively used for direct instantiation of PositiveIntegerItemImpl in IPositiveIntegerItem.java. No factory methods or builders are present, confirming that the visibility change is intentional.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any factory methods or builders in the codebase that should be used instead
ast-grep --pattern 'class $_ {
$$$
static $_ create$_(PositiveIntegerItemImpl|BigInteger) {
$$$
}
$$$
}'
# Check for direct instantiations to understand usage pattern
rg "new PositiveIntegerItemImpl\(" -A 2
Length of output: 509
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateTimeWithTimeZoneItemImpl.java (2)
Line range hint 6-13: LGTM! Clean package structure and imports.
The move to an 'impl' subpackage improves separation of concerns, and the imports are well-organized with appropriate use of nullability annotations.
Line range hint 32-41: Verify type adapter singleton usage.
The implementation looks correct, but let's verify the singleton pattern usage of MetaschemaDataTypeProvider.DATE_TIME_WITH_TZ across the codebase.
✅ Verification successful
Singleton Pattern Usage Verified
The MetaschemaDataTypeProvider.DATE_TIME_WITH_TZ is consistently used as a singleton across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of the DATE_TIME_WITH_TZ adapter
# Expected: All usages should reference the singleton instance
# Search for any direct instantiation of DateTimeWithTZAdapter
rg "new DateTimeWithTZAdapter"
# Search for all references to DATE_TIME_WITH_TZ
rg "DATE_TIME_WITH_TZ" -A 2
Length of output: 5937
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/NonNegativeIntegerItemImpl.java (2)
20-22: LGTM! Class structure is well-designed
The class hierarchy and interface implementation are appropriate for the domain model.
20-38: Verify integration with related changes
Based on the AI summary, several related changes were made to type handling and casting. Let's verify the integration:
✅ Verification successful
Verification successful: No issues found with the integration and interface usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify related changes and usage patterns
# Check for any remaining cast operations in type adapters
echo "Checking for cast operations in type adapters..."
rg -p "cast.*Integer" --type java
# Check for usage in FunctionUtils
echo "Checking usage in FunctionUtils..."
ast-grep --pattern 'class FunctionUtils {
$$$
$_($_, NonNegativeIntegerItemImpl $_) {
$$$
}
$$$
}'
# Check for proper interface usage
echo "Checking interface usage patterns..."
rg -p "INonNegativeIntegerItem" --type java
Length of output: 3059
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateWithoutTimeZoneItemImpl.java (1)
Line range hint 30-41: Implementation looks good!
The code follows good practices with focused methods and clear responsibilities.
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupDataTypeProvider.java (1)
Line range hint 1-42: Verify handling of casting operations after refactoring.
Given that casting operations were relocated to atomic item implementations (as mentioned in PR objectives), we should verify that this provider properly integrates with the new casting mechanism.
✅ Verification successful
Casting operations correctly relocated to atomic item implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining casting operations in related adapter classes
echo "Checking for casting operations in markup adapters..."
rg -l "cast\(" --type java $(fd -e java "Markup.*Adapter\.java$")
# Verify the new location of casting operations
echo "Verifying casting operations in atomic items..."
rg -l "cast\(" --type java $(fd -e java "Markup.*Item\.java$")
Length of output: 816
core/src/main/java-templates/gov/nist/secauto/metaschema/core/MetaschemaJavaVersion.java (1)
11-17: LGTM! Good encapsulation improvement.
Converting the fields from public to private is a positive change that:
- Improves encapsulation by preventing direct access to the template variables
- Enforces access through the getter methods defined by IVersionInfo
- Maintains better control over version information access
The change is safe since all necessary access is already provided through the interface methods.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/IPv4AddressItemImpl.java (3)
6-14: LGTM! Good separation of implementation details.
The move to the .impl package follows good practices by separating interface from implementation.
20-22: LGTM! Proper inheritance hierarchy.
The class correctly extends AbstractUntypedAtomicItem with proper generic type and implements the appropriate interface.
Line range hint 34-42: LGTM! Methods are concise and efficient.
Both methods are well-implemented:
- getJavaTypeAdapter() efficiently uses a singleton instance
- asIpAddress() provides a clean type-safe access to the value
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/IPv6AddressItemImpl.java (2)
6-14: LGTM! Package structure follows good practices.
The move to the .impl subpackage properly separates implementation from interface, following the Interface Segregation Principle.
20-22: LGTM! Class structure is well-designed.
The inheritance hierarchy and generic type usage provide proper type safety and follow good object-oriented design principles.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/MarkupLineItemImpl.java (4)
6-14: LGTM! Package structure follows good separation of concerns.
The move to the impl package properly separates the implementation from the interface, following good architectural practices.
20-22: LGTM! Class structure is well-designed.
The inheritance hierarchy and interface implementation are appropriate, with proper use of generics.
24-32: LGTM! Constructor is well-implemented and documented.
The constructor properly enforces non-null constraints and is well-documented.
Line range hint 20-43: Verify consistency with other atomic item implementations.
As part of the broader refactoring effort, let's ensure this implementation follows the same patterns as other atomic item implementations.
✅ Verification successful
Consistent with other atomic item implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other atomic item implementations to verify consistency
ast-grep --pattern 'class $_ItemImpl
extends AbstractUntypedAtomicItem<$_>
implements $_Item {
$$$
}'
Length of output: 113
Script:
#!/bin/bash
# Search for classes extending AbstractUntypedAtomicItem
rg 'class\s+\w+\s+extends\s+AbstractUntypedAtomicItem' --context 2
Length of output: 68
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/MarkupMultiLineItemImpl.java (3)
6-12: LGTM! Good separation of implementation details.
Moving the implementation to the .impl package improves encapsulation and follows the interface-implementation separation pattern.
24-32: LGTM! Well-documented constructor with proper null-safety.
The constructor is properly documented and uses @nonnull annotation for compile-time null checking.
16-22: Verify complete implementation of IMarkupItem interface.
The class structure and documentation look good. Let's verify all required interface methods are implemented.
core/src/main/java-templates/gov/nist/secauto/metaschema/core/model/MetaschemaVersion.java (2)
12-18: LGTM! Good encapsulation improvement
Converting the fields from public to private improves encapsulation while still providing controlled access through the interface methods.
27-27: Verify impact of version reporting change
The method now returns BUILD_VERSION instead of CLOSEST_TAG. While this provides more stable versioning, please verify that this change doesn't break existing consumers who might depend on git tag versioning.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractDateItem.java (4)
6-9: LGTM: Good separation of interface and implementation
Moving the class to the impl package follows good design practices by maintaining a clear separation between interface and implementation.
38-41: LGTM: Clean implementation of castAsType
The implementation correctly delegates to the interface's static cast method, maintaining a clean separation of concerns.
52-52: LGTM: Clean equals implementation
The simplified instanceof check and use of compareTo for equality comparison is correct and follows best practices for implementing equals.
38-41: Verify consistent implementation across IDateItem implementations
Let's ensure all IDateItem implementations follow the same pattern for castAsType.
✅ Verification successful
All IDateItem implementations have consistent castAsType implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent implementation of castAsType across IDateItem implementations
# Find all implementations of IDateItem
echo "Searching for IDateItem implementations..."
ast-grep --pattern 'class $_ implements $$$IDateItem$$$'
# Check castAsType implementations
echo "Checking castAsType implementations..."
ast-grep --pattern 'public IDateItem castAsType(IAnyAtomicItem item) {
$$$
}'
Length of output: 322
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractDateTimeItem.java (2)
6-9: LGTM! Clean package structure and imports.
The package organization and imports are well-structured and follow Java conventions.
38-41: Verify the cast implementation across the codebase.
The castAsType implementation correctly delegates to the interface's static method. Let's verify the usage pattern across the codebase.
core/src/test/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnCeilingTest.java (1)
9-9: LGTM: Import addition is appropriate
The added import for the integer static method aligns with the test modifications and follows proper import organization.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractUriItem.java (2)
6-6: LGTM! Package restructuring improves encapsulation
The move to the .impl package and the addition of appropriate imports better separates implementation details from the public API.
Also applies to: 9-10
55-55: LGTM! Improved readability in equals method
The removal of unnecessary parentheses improves code readability while maintaining the correct functionality. The equals implementation correctly follows the pattern for comparable objects.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractIntegerItem.java (3)
6-8: LGTM! Good separation of implementation from API
Moving the implementation class to an .impl package improves the separation between the API and its implementation details.
63-64: LGTM! Clean equals implementation
The equals method implementation is correct and follows the equals contract. The removal of unnecessary parentheses improves readability while maintaining the same logical operation.
Line range hint 6-64: Verify consistency with other IIntegerItem implementations
As part of the broader refactoring effort, let's ensure consistency across all implementations.
✅ Verification successful
Consistency Verified
All IIntegerItem implementations are consistent with AbstractIntegerItem.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all implementations of IIntegerItem
echo "Searching for IIntegerItem implementations..."
ast-grep --pattern 'class $_ implements $$$IIntegerItem$$$'
# Find all classes that extend AbstractIntegerItem
echo -e "\nSearching for AbstractIntegerItem subclasses..."
ast-grep --pattern 'class $_ extends AbstractIntegerItem'
# Find usages of AbstractIntegerItem
echo -e "\nSearching for AbstractIntegerItem usages..."
rg "AbstractIntegerItem" -A 2
Length of output: 2429
Script:
#!/bin/bash
# Extract method declarations and overrides in subclasses of AbstractIntegerItem
# Define the list of subclasses
subclasses=(
"core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/PositiveIntegerItemImpl.java"
"core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/NonNegativeIntegerItemImpl.java"
"core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/IntegerItemImpl.java"
)
# Loop through each subclass and extract method signatures
for subclass in "${subclasses[@]}"; do
echo "Extracting methods from $subclass..."
ast-grep --pattern 'method $_($_) { $$$ }' "$subclass"
done
Length of output: 1719
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractTemporalItem.java (2)
Line range hint 14-19: LGTM! Documentation improvements enhance clarity.
The updated class documentation better describes the class's purpose as a base for all ITemporalItem-derived items. The class structure maintains good abstraction and follows proper OOP principles.
6-11: Verify package move impact across the codebase.
The class has been moved to the .impl package, which is a good practice for implementation classes. However, we should ensure all references to this class have been updated accordingly.
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/UriAdapter.java (1)
27-27: Verify adapter registration
Let's confirm that this adapter is properly registered in the MetaschemaDataTypeProvider.
✅ Verification successful
Adapter registration verified
UriAdapter is properly registered in MetaschemaDataTypeProvider.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UriAdapter registration in MetaschemaDataTypeProvider
rg -l "UriAdapter" "core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/MetaschemaDataTypeProvider.java"
# Check the pattern of registration
ast-grep --pattern 'class MetaschemaDataTypeProvider {
$$$
public static final UriAdapter URI = $_;
$$$
}'
Length of output: 335
Script:
#!/bin/bash
# List all lines containing 'UriAdapter' in MetaschemaDataTypeProvider.java
grep "UriAdapter" "core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/MetaschemaDataTypeProvider.java"
# Search for registration methods involving 'UriAdapter'
rg "register.*UriAdapter" "core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/MetaschemaDataTypeProvider.java"
Length of output: 304
Script:
#!/bin/bash
# Find all occurrences of 'URI' in MetaschemaDataTypeProvider.java
grep "URI" "core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/MetaschemaDataTypeProvider.java"
# Find all references to 'UriAdapter' across the codebase
rg "UriAdapter" "core/src/main/java/gov/nist/secauto/metaschema/"
Length of output: 1058
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/Base64BinaryItemImpl.java (2)
6-16: LGTM! Good package organization.
Moving the implementation to an 'impl' package follows good practice by separating implementation from interface definition.
22-34: LGTM! Well-structured class definition.
Good use of:
- Proper inheritance hierarchy
- Constructor documentation
- Null-safety annotations
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/UuidAdapter.java (2)
23-27: LGTM! Well-documented class purpose
The added class-level documentation clearly describes the purpose and includes a valuable reference to the specification.
33-34: 🛠️ Refactor suggestion
Consider removing the unused UUID_PATTERN field
The UUID_PATTERN field appears to be unused since the parse method relies on UUID.fromString() for validation. The presence of multiple warning suppressions (unused and PMD.UnusedPrivateField) further suggests this field is not necessary.
Consider removing the field entirely:
- @SuppressWarnings({ "unused", "PMD.UnusedPrivateField" })
- private static final Pattern UUID_PATTERN
- = Pattern.compile("^[0-9A-Fa-f]{8}-[0-9A-Fa-f]{4}-[45][0-9A-Fa-f]{3}-[89ABab][0-9A-Fa-f]{3}-[0-9A-Fa-f]{12}$");Let's verify if this pattern is used elsewhere in the codebase:
✅ Verification successful
UUID_PATTERN Field is Unused and Can Be Removed
The UUID_PATTERN field is not used anywhere else in the codebase.
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/UuidAdapter.java(Lines 33-34)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any usage of UUID_PATTERN
rg "UUID_PATTERN" --type java
Length of output: 159
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DayTimeDurationItemImpl.java (2)
6-16: LGTM! Package restructuring improves code organization.
The move to the impl package better separates the implementation from the interface, following good design practices.
22-34: LGTM! Well-structured class implementation.
The class hierarchy, constructor implementation, and nullability constraints are properly defined.
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DecimalAdapter.java (2)
25-29: Well-structured documentation!
The added Javadoc provides clear documentation with proper linking to the specification.
32-32: Good encapsulation improvement!
Making MATH_CONTEXT private is a good practice since it's only used internally within the parse() method. This change aligns with the principle of encapsulation.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/ITokenItem.java (4)
9-9: LGTM: Import addition aligns with enhanced error handling
The addition of InvalidTypeMetapathException import supports the improved error handling strategy implemented in the interface.
15-17: LGTM: Documentation improvements enhance clarity
The added interface documentation and updated @throws clause provide clear information about the interface's purpose and error conditions.
Also applies to: 25-26
54-60: LGTM: Well-structured cast implementation
The refactored cast implementation follows good practices:
- Efficient direct casting for compatible types
- Clean fallback to string conversion
- Proper exception handling and wrapping
33-38: Consider providing context information in exception constructor
The null parameter in the InvalidTypeMetapathException constructor might be missing important context information. Consider providing relevant context (e.g., the current item or operation context) to help with debugging.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/YearMonthDurationItemImpl.java (2)
6-18: LGTM! Good package organization.
Moving the implementation to the .impl package follows good separation of concerns principles by isolating implementation details from the interface.
23-35: LGTM! Well-structured class definition and constructor.
Good use of @NonNull annotation to enforce null safety, and the constructor documentation clearly describes its purpose.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractStringItem.java (2)
6-6: LGTM! Well-structured package organization and documentation.
The package restructuring and added documentation improve code organization and clarity. The class-level Javadoc clearly describes the purpose of this abstract class.
Also applies to: 9-10, 17-19
57-57: LGTM! Clean equals implementation.
The simplified condition in the equals method follows best practices by:
- Removing unnecessary parentheses
- Maintaining proper type checking
- Following the standard equals contract
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/UuidItemImpl.java (3)
6-18: LGTM: Package restructuring improves separation of concerns
The move to the .impl subpackage better separates the implementation from the interface, following good design practices.
27-34: LGTM: Well-documented constructor with proper null checking
The constructor is properly documented and includes appropriate null-safety annotation.
64-67: LGTM: Simplified equals implementation
The removal of unnecessary parentheses improves readability while maintaining the same logic.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DecimalItemImpl.java (5)
6-10: LGTM!
Package organization and imports are well-structured.
18-21: LGTM!
Clear and concise class documentation that follows JavaDoc conventions.
23-26: LGTM! Well-implemented constants.
Good practices followed:
- Using string literals for BigDecimal initialization prevents floating-point precision issues
- Proper use of @nonnull annotation
- Constants are appropriately encapsulated
82-83: LGTM! Clean equals implementation.
The simplified instanceof check improves readability while maintaining correct equality comparison behavior.
28-38: LGTM! Verify usage patterns across the codebase.
The implementation is clean and well-documented. Let's verify how this new utility method is being used.
✅ Verification successful
Usage of toBigDecimal method is correctly implemented and utilized appropriately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage patterns of toBigDecimal method
# Look for potential callers of this method to ensure it's being utilized appropriately
# Search for direct method calls
echo "Checking for direct method calls to toBigDecimal:"
rg "DecimalItemImpl\.toBigDecimal\(" --type java
# Search for potential boolean to decimal conversions that might benefit from this method
echo "Checking for potential boolean to decimal conversions:"
ast-grep --pattern 'new BigDecimal($expr)' --lang java
Length of output: 483
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IEmailAddressItem.java (4)
15-17: LGTM! Clear and concise documentation.
The added class-level documentation clearly describes the purpose of the interface.
24-26: LGTM! Documentation accurately reflects error handling.
The updated documentation properly describes the new exception type and its conditions.
54-61: LGTM! Robust error handling implementation.
The cast method implementation shows good practices:
- Efficient type checking
- Proper exception handling with multiple catch types
- Helpful inline documentation about potential exceptions
- Correct exception chaining
33-37: Verify the null parameter usage in InvalidTypeMetapathException constructor.
The first parameter of the InvalidTypeMetapathException constructor is set to null. This might need attention if it represents important context information.
✅ Verification successful
The usage of null as the first parameter in InvalidTypeMetapathException is consistent with existing codebase usages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other InvalidTypeMetapathException usages to understand the expected first parameter
ast-grep --pattern 'new InvalidTypeMetapathException($_, $_, $_)'
Length of output: 16729
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/Base64Adapter.java (1)
23-27: LGTM! Well-documented class purpose.
The added Javadoc clearly describes the purpose of the class and provides a helpful link to the specification.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IAnyAtomicItem.java (1)
10-10: LGTM! Documentation improvements enhance API clarity
The addition of InvalidValueForCastFunctionException import and the enhanced documentation for castAsType method improve the API contract's clarity by explicitly documenting the error conditions and return type.
Also applies to: 84-86
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv6AddressItem.java (2)
9-11: LGTM! Import changes align with new error handling.
The added imports support the enhanced error handling in the valueOf and cast methods.
66-73: LGTM! Robust implementation of the cast method.
The changes improve type safety and error handling:
- Efficient fast path for existing IPv6 items
- Proper exception handling with clear context
- Helpful comment explaining the IllegalStateException source
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv4AddressItem.java (2)
9-11: LGTM! Import changes align with new functionality.
The new imports support the enhanced error handling and implementation requirements.
21-40: LGTM! Well-structured valueOf implementation with proper error handling.
The method is well-documented and properly handles parsing errors while maintaining type safety.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUriReferenceItem.java (1)
9-11: LGTM! Appropriate exception imports added
The new imports support the enhanced error handling capabilities added to the interface.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/BooleanItemImpl.java (4)
34-41: LGTM! Well-documented constructor
The constructor documentation is clear and follows standard Javadoc conventions. The parameter rename to value is more concise since the type already indicates it's a boolean.
65-65: Good improvement in type safety
The change from IDataTypeAdapter<?> to IDataTypeAdapter<Boolean> eliminates the need for type casting and provides better compile-time type checking.
88-88: LGTM! Clean equals implementation
The simplified instanceof check maintains readability while following proper equals contract implementation.
105-106: LGTM! Improved readability
The multi-line formatting of the equals condition improves code readability while maintaining proper equals contract implementation.
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateTimeAdapter.java (3)
26-30: LGTM! Well-documented class purpose.
The added Javadoc with the link to the specification is helpful for developers.
79-81: LGTM! Good simplification with null safety.
The method has been effectively simplified while maintaining null safety through ObjectUtils.notNull().
Line range hint 1-94: Verify the impact of removing castInternal method.
The removal of castInternal method is part of a larger refactoring effort. Let's verify that all type conversion scenarios are properly handled by the new implementation.
✅ Verification successful
Removal of castInternal method verified successfully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to castInternal in the codebase
rg "castInternal.*IDateTimeItem"
# Look for any type conversion calls that might be affected
ast-grep --pattern 'class $_ extends AbstractCustomJavaDataTypeAdapter {
$$$
cast($_) {
$$$
}
$$$
}'
Length of output: 149
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUuidItem.java (2)
9-11: LGTM! Import changes align with new error handling.
The added imports support the enhanced error handling capabilities introduced in this interface.
Line range hint 21-86: Verify consistency with other atomic item implementations.
As this is part of a broader refactoring effort, let's ensure the error handling pattern is consistent across other atomic item implementations.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBase64BinaryItem.java (2)
9-11: LGTM! Import changes align with enhanced error handling.
The new exception imports support the improved error handling strategy.
29-31: LGTM! Clear and informative exception documentation.
The updated method signature and Javadoc properly document the new exception handling behavior.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDayTimeDurationItem.java (2)
17-20: Well-documented interface purpose!
The added documentation clearly describes the interface's purpose and its relationship to duration data values.
71-78: Verify consistent casting pattern across other atomic item types
The casting implementation is robust with proper error handling and type checking. This appears to be part of a larger refactoring effort to centralize casting operations.
Let's verify this pattern is consistently implemented across other atomic item types:
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IPositiveIntegerItem.java (5)
10-10: LGTM!
The import of the implementation class is appropriate for the factory methods.
16-18: LGTM!
Clear and concise interface documentation that follows Javadoc standards.
110-119: LGTM! Well-structured type casting implementation.
The implementation follows a logical type checking hierarchy with proper error handling and exception chaining. The comment explaining the IllegalStateException case is helpful.
124-124: LGTM! Efficient implementation.
The simplified implementation removes unnecessary conversion steps while maintaining type safety.
60-60: Verify handling of non-integer numeric types.
The parameter type change from IIntegerItem to INumericItem broadens the accepted input types. Please ensure that the asInteger() method properly handles conversion of non-integer numeric types (e.g., decimals).
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IAnyUriItem.java (5)
9-11: LGTM! Import changes align with enhanced error handling.
The new imports support the improved exception handling in the valueOf and cast methods.
18-20: LGTM! Clear and concise interface documentation.
The added Javadoc properly describes the interface's purpose.
76-84: LGTM! Robust cast implementation with proper error handling.
The implementation follows best practices:
- Attempts direct cast first
- Falls back to string conversion
- Handles all potential exceptions appropriately
- Includes helpful documentation about exception scenarios
86-89: LGTM! Clean and correct default implementation.
The default implementation properly delegates to the cast method, following the interface segregation principle.
Line range hint 1-150: Verify consistency with other atomic item implementations.
The changes to error handling and type safety look good, but we should verify that similar patterns are followed in other atomic item implementations.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INumericItem.java (1)
38-45: Verify handling of special numeric values.
The implementation looks good and efficiently handles direct casting and string conversion. However, let's verify the handling of edge cases.
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/IMarkupString.java (1)
29-34: Well-structured interface documentation!
The added Javadoc clearly describes the interface's purpose and properly documents the type parameter. This addition improves code documentation and maintainability.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnMinMax.java (1)
18-23: LGTM! Imports are well-organized and support the new functionality.
The new imports for atomic item types are necessary for the enhanced type handling system.
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/MetaschemaDataTypeProvider.java (3)
174-180: LGTM! Default data type is well defined.
The default data type is properly set to STRING and well documented.
156-159: Verify migration path for deprecated NCNAME adapter.
The NCNAME adapter is marked as deprecated. Please ensure there's a clear migration path for existing users.
#!/bin/bash
# Search for usages of the deprecated NCNAME adapter
echo "Searching for NCNAME usage in code..."
rg --type java "NCNAME" -B 2 -A 2
# Search for migration documentation
echo "Checking for migration documentation..."
fd -e md -e txt . | xargs rg -l "NCNAME"Line range hint 19-213: Verify adapter class implementations.
Please ensure all referenced adapter classes are properly implemented and follow consistent patterns.
✅ Verification successful
All adapter classes are properly implemented and registered.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existence and implementation of all adapter classes
echo "Checking adapter implementations..."
for adapter in Base64Adapter BooleanAdapter DateAdapter DateWithTZAdapter DateTimeAdapter \
DateTimeWithTZAdapter IPv4AddressAdapter IPv6AddressAdapter UriAdapter \
UriReferenceAdapter UuidAdapter DayTimeAdapter YearMonthAdapter DecimalAdapter \
IntegerAdapter NonNegativeIntegerAdapter PositiveIntegerAdapter EmailAddressAdapter \
HostnameAdapter NcNameAdapter StringAdapter TokenAdapter; do
echo "Checking $adapter..."
# Search for adapter class definitions
rg "class $adapter" --type java
# Verify basic required methods exist
rg "parse|format|asString" --type java -l | rg "$adapter"
done
Length of output: 17963
core/src/main/java/gov/nist/secauto/metaschema/core/model/validation/XmlSchemaContentValidator.java (1)
60-63: LGTM: Constructor changes are consistent and well-documented.
The constructor signature change from SAXException to IOException is consistent with the error handling strategy in toSchema. The null checking and documentation are properly maintained.
schemagen/src/test/java/gov/nist/secauto/metaschema/schemagen/AbstractSchemaGeneratorTestSuite.java (1)
125-127: Verify the impact of removing SAXException handling
The removal of SAXException from the catch block might lead to unhandled XML-specific errors during schema validation. While this aligns with the PR's objective to simplify error handling, we should ensure that:
- XML parsing errors are properly propagated
- The calling code can handle these errors appropriately
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/IDataTypeAdapter.java (1)
144-156: LGTM! Removal of cast method improves separation of concerns.
The removal of the cast method from IDataTypeAdapter is a positive change that:
- Aligns with the PR objective of relocating casting operations to atomic item implementations
- Improves separation between type adapters and Metapath items
- Reduces the responsibilities of the
IDataTypeAdapterinterface
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/FunctionUtils.java (3)
130-132: LGTM: Improved null handling with clear error message
The addition of an explicit null check with a descriptive error message enhances error handling and prevents potential NullPointerException.
243-244: LGTM: Improved code readability
The simplification of the conditional logic makes the code more maintainable while preserving the same functionality.
146-146: Verify impact of @nonnull annotation change
The parameter annotation change from @nullable to @nonnull is a breaking change that could affect existing callers. Let's verify there are no null values being passed to this method.
✅ Verification successful
@nonnull Annotation Change Verified
No calls to toNumeric pass null values, ensuring the change to @nonnull is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct calls to toNumeric with null or potentially null values
rg -l "toNumeric\(\s*null\s*\)" || \
rg -l "toNumeric\(\s*[^,\s]+\s*\)" | while read -r file; do
echo "=== $file ==="
# Show the context of the calls
rg -B 2 -A 2 "toNumeric\(" "$file"
done
Length of output: 4386
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/ComparisonFunctions.java (1)
153-153: LGTM! Casting operation change aligns with refactoring goals
The change from item.getJavaTypeAdapter().cast(other) to item.castAsType(other) successfully moves the casting responsibility to the atomic item implementation, improving the separation between type adapters and Metapath items.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPAddressItem.java (2)
13-15: Well-documented interface with Javadoc comments
The added Javadoc comments provide a clear and concise description of the IIPAddressItem interface, improving code readability and maintainability.
8-9: Import statement is appropriate
The import of InvalidValueForCastFunctionException is necessary for the new cast method's exception handling.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IHostnameItem.java (2)
31-35: Consistent exception type in valueOf method
The change to throw InvalidTypeMetapathException enhances consistency in error handling when the input value is not a valid hostname.
52-59: Improved exception handling in cast method
The cast method now appropriately handles exceptions by rethrowing InvalidValueForCastFunctionException, ensuring consistent error reporting for invalid casts.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INcNameItem.java (1)
9-11: Appropriate addition of necessary imports
The imports for InvalidTypeMetapathException and NcNameItemImpl are correctly added to support the updated exception handling and class instantiation.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IStringItem.java (5)
8-9: Imports added appropriately
The new imports are necessary for the added functionality and are correctly included.
11-11: Addition of StringItemImpl import
Importing StringItemImpl is appropriate for instantiating string items.
15-17: Enhanced Javadoc comment
The added Javadoc improves code documentation and clarity.
53-55: Casting logic is correctly implemented
The casting logic properly handles instances of IStringItem and calls valueOf when necessary.
57-58: Exception handling in cast method
Catching IllegalStateException from asString() and rethrowing as InvalidValueForCastFunctionException ensures proper exception management.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDateItem.java (3)
38-43: Improved exception handling in valueOf method.
Changing the exception to InvalidTypeMetapathException provides a more specific and informative error when parsing invalid date strings, enhancing error reporting.
85-102: Robust type casting in cast method.
The refactored cast method now handles various atomic item types more effectively, ensuring correct casting to IDateItem. The use of specific exceptions improves error clarity.
106-106:
Potential breaking change: castAsType method now requires implementation.
By removing the default implementation of castAsType(IAnyAtomicItem item) in the IDateItem interface, any implementing classes without an explicit implementation will fail to compile. Please ensure all classes implementing IDateItem provide an implementation for this method to avoid compilation errors.
Run the following script to identify classes implementing IDateItem that may lack the castAsType method:
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapter.java (1)
92-94: Approved: Simplified return statement enhances readability
The refactored return statement in the asString method improves readability by directly returning the formatted date string without unnecessary variables.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IYearMonthDurationItem.java (5)
8-9: Import InvalidTypeMetapathException is appropriate and necessary
The addition of the import statement for InvalidTypeMetapathException is correct, as the exception is now used within the class.
10-11: Ensure YearMonthDurationItemImpl is correctly utilized
The import of YearMonthDurationItemImpl suggests its usage in the implementation. Verify that this implementation class correctly adheres to the interface's contract.
29-31: Javadoc updated to reflect thrown exception
The Javadoc correctly documents the new InvalidTypeMetapathException that can be thrown, which improves the accuracy of the API documentation.
72-72: Null check ensures non-null Period instance
Using ObjectUtils.notNull ensures that the Period instance is not null, which enhances the robustness of the method.
87-94: Verify consistency with other cast methods in the codebase
Ensure that the casting approach used here aligns with other cast methods across different atomic item interfaces for consistency.
Run the following script to compare cast methods:
✅ Verification successful
Casting approach is consistent with other atomic item interfaces
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find and display all `cast` methods in atomic item interfaces.
# Search for `cast` methods in interfaces extending `IAnyAtomicItem`
fd -e java 'I*.java' --exec grep -H -A 5 'static.*cast(' {}
Length of output: 19777
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBooleanItem.java (2)
39-40: Javadoc Updated with New Exception
The addition of @throws InvalidTypeMetapathException in the Javadoc accurately documents the new exception that can be thrown by the valueOf(String value) method. This enhances the API documentation and informs users of potential exceptions.
130-130: Verify Type Compatibility in compareTo Implementation
The compareTo(IAnyAtomicItem item) method casts the input using castAsType(item). It's important to ensure that all possible item instances are compatible with this cast to prevent ClassCastException at runtime.
Run the following script to identify potential issues with type casting:
Review the results to ensure that all usages are type-safe and handle incompatible types appropriately.
✅ Verification successful
Type Compatibility Verified Successfully
All compareTo usages in IBooleanItem ensure type safety by verifying instance types before casting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to IBooleanItem.compareTo and check for type safety.
# Search for usages of IBooleanItem.compareTo
rg -t java 'IBooleanItem\s+.*compareTo\s*\('
# Check for implementations of compareTo that might accept incompatible types
rg -A 3 'int compareTo\s*\(.*\)' -t java
Length of output: 14098
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIntegerItem.java (2)
133-149: Casting logic in cast method is comprehensive
The updated cast method appropriately handles casting from IIntegerItem, INumericItem, and IBooleanItem. The exception handling ensures that invalid casts are properly managed.
151-153: castAsType override maintains consistency
Overriding castAsType to call cast(item) directly ensures consistent casting behavior and simplifies the codebase.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDecimalItem.java (6)
9-11: Appropriate addition of imports for new exception and implementation
The added import statements for InvalidTypeMetapathException and DecimalItemImpl are necessary for the updated exception handling and method implementations.
35-36: Correct update of exception type in documentation
The Javadoc now correctly specifies that InvalidTypeMetapathException is thrown when the input string is not a valid decimal.
43-47: Enhanced exception handling with specific exception
The code now throws InvalidTypeMetapathException with a detailed message when parsing fails, improving error specificity and clarity.
76-87: Addition of valueOf(boolean value) method
The new method allows creation of an IDecimalItem from a boolean value, enhancing functionality. Ensure that DecimalItemImpl.toBigDecimal(boolean) correctly converts boolean values to the expected decimal representations.
111-127: Improved cast method with comprehensive type handling
The cast method now properly handles casting from various atomic item types, including IDecimalItem, INumericItem, and IBooleanItem, enhancing type safety and correctness.
132-132: Simplification of castAsType method
The castAsType method now directly returns the result of cast(item), simplifying the code and reducing redundancy.
...a/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/NonNegativeIntegerItemImpl.java
Show resolved
Hide resolved
...in/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/Base64BinaryItemImpl.java
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/Base64Adapter.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv6AddressItem.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv4AddressItem.java
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapter.java
Show resolved
Hide resolved
...main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java
Show resolved
Hide resolved
...main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java
Show resolved
Hide resolved
.../main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IYearMonthDurationItem.java
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBooleanItem.java
Show resolved
Hide resolved
…s to provide more details.
…y and resulting time value.
david-waltermire
force-pushed
the
feature/code-cleanup-20241111
branch
from
b00a487
to
9a4237f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (82)
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/NonMessageExitStatus.java (3)
10-12: Enhance class-level documentation for better clarity.Consider expanding the Javadoc to include:
- When to use this implementation over other ExitStatus variants
- Its relationship with AbstractExitStatus
- The @SInCE tag to indicate when this class was introduced
/** - * An {@link ExitStatus} implementation without an associated message. + * An {@link ExitStatus} implementation that represents a status without an associated message. + * This implementation is useful when only the exit code needs to be communicated, without + * additional context or explanation. + * + * @since 1.0.0 */
Line range hint
17-19: Improve constructor documentation.The constructor's Javadoc should document the parameter and any constraints.
/** - * Construct a new message status. + * Construct a new exit status without an associated message. + * + * @param code the non-null exit code representing the status + * @throws NullPointerException if the provided code is null */
Line range hint
21-24: Convert inline comment to proper Javadoc.Consider converting the inline comment to a formal Javadoc for better API documentation.
+ /** + * @return {@code null} as this implementation does not support messages + */ @Override protected String getMessage() { - // always null return null; }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/FailingErrorListener.java (1)
13-19: Documentation looks good, consider adding parameter documentation.The added Javadoc clearly describes the class purpose and behavior. However, it would be beneficial to document the parameters of the
syntaxErrormethod using@paramtags.Consider enhancing the documentation:
/** * An ANTLR error listener that throws a {@link ParseCancellationException} when * a syntax error is found. * <p> * The exception message contains details around the line and character position * where the error occurred. * + * @see BaseErrorListener#syntaxError(Recognizer, Object, int, int, String, RecognitionException) */cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/impl/DefaultExtraArgument.java (1)
12-14: Enhance class documentation for better clarity.The current documentation is minimal. Consider expanding it to include:
- The purpose and typical usage of this implementation
- Its role in the command processing system
- Any specific behaviors or constraints
/** - * This implementation of an extra argument. + * A default implementation of the {@link ExtraArgument} interface that represents + * a named command-line argument which can be marked as required or optional. + * This implementation is used by the command processor to handle additional + * arguments that are not covered by specific command options. */core/src/main/java/gov/nist/secauto/metaschema/core/datatype/object/AmbiguousDate.java (1)
36-37: Consider adding @nonnull annotation to the copy method.The copy method implementation is correct, but could benefit from explicitly declaring its non-null contract.
@Override + @NonNull public AmbiguousDate copy() { return new AmbiguousDate(getValue(), hasTimeZone()); }cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/ExitStatus.java (1)
24-30: LGTM! Well-designed error handling enhancement.The new
getThrowable()method is a good addition that:
- Complements the existing
withThrowable()method for complete error handling- Uses appropriate nullability annotations
- Has clear documentation including null behavior
This enhancement improves error handling by allowing consumers to:
- Access error details without processing them (via
getThrowable())- Process errors with stack trace control (via
generateMessage())- Chain error handling (via
withThrowable())core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateWithoutTimeZoneItemImpl.java (2)
16-21: Consider enhancing the class documentation.While the current documentation is good, it could be even more helpful by including an example of when/why a date might not have an explicit timezone.
Add to the class Javadoc:
/** * An implementation of a Metapath atomic item containing a date data value that * may not have an explicit timezone. + * + * <p>For example, when parsing dates from data sources that don't specify timezone + * information, such as "2024-01-01". */
33-41: Add Javadoc for overridden methods.While the implementation is correct, adding documentation for the overridden methods would improve maintainability.
Add method documentation:
+ /** + * {@inheritDoc} + */ @Override public ZonedDateTime asZonedDateTime() { return getValue().getValue(); } + /** + * {@inheritDoc} + */ @Override public DateAdapter getJavaTypeAdapter() { return MetaschemaDataTypeProvider.DATE; }cli-processor/src/main/java-templates/gov/nist/secauto/metaschema/cli/processor/ProcessorVersion.java (2)
7-13: Consider adding @SInCE tag to the class documentation.The Javadoc is well-written, but adding an
@sincetag would help track API versioning and maintain better documentation./** * Provides version information for this library. * <p> * This class exposes build-time metadata including version numbers, build * timestamps, and Git repository information. + * + * @since 1.0.0 */
15-15: Consider making NAME configurable via build properties.While other constants use template variables,
NAMEis hardcoded. Consider making it configurable through Maven properties for consistency and flexibility.- private static final String NAME = "metaschema-java"; + private static final String NAME = "${project.name}";cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/CommandService.java (2)
18-22: Consider enhancing class-level documentation.While the current documentation is good, consider adding information about the singleton pattern usage, as it's a crucial aspect of this class's design.
/** * A service that loads commands using SPI. * + * This class implements the singleton pattern to ensure a single instance of the command service + * is used throughout the application. * * @see ServiceLoader for more information */
37-39: Consider adding more details to constructor documentation.The constructor documentation could be more informative by explaining the ServiceLoader initialization.
/** * Construct a new service. + * + * Initializes the ServiceLoader for ICommand implementations. + * This constructor is private to enforce the singleton pattern. */cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/MessageExitStatus.java (2)
16-18: Consider enhancing class documentationWhile the current documentation is good, consider mentioning that message arguments are stored immutably, as this is an important implementation detail for users of this class.
/** * An {@link ExitStatus} implementation with an associated message. + * The message arguments are stored in an unmodifiable list to ensure thread-safety + * and immutability. */
Line range hint
44-44: Consider implementing message bundle supportThe TODO comment indicates a planned enhancement for message bundle support. This would be valuable for internationalization.
Would you like me to create a GitHub issue to track the implementation of message bundle support?
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/AstCollectingVisitor.java (3)
6-6: Good architectural decision moving implementation toimplpackage.Moving implementation classes to an
implpackage follows Java best practices for encapsulation and API design. This change clearly separates implementation details from the public API.
15-18: Consider enhancing class documentation.While the added documentation is good, consider adding:
@sincetag to track API versioning- Details about the format of the text-based representation
- Example output to help users understand the tree visualization
/** * This class supports building a text-based representation of a Markup node * tree. + * + * The tree is represented with indentation to show parent-child relationships, + * where each level is indented by two spaces. + * + * Example output: + * Node1 + * Child1 + * Child2 + * GrandChild1 + * + * @since 1.0.0 */
21-21: Good improvement for cross-platform compatibility.Using
System.lineSeparator()instead of hardcoded "\n" is a better approach for cross-platform compatibility. Consider renaming the constant to be more descriptive:- private static final String EOL = System.lineSeparator(); + private static final String LINE_SEPARATOR = System.lineSeparator();core/src/test/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapterTest.java (1)
26-33: Consider adding more edge cases to test data.While the current test cases cover basic scenarios well, consider adding:
- Dates with time components (HH:mm:ss)
- Dates with fractional seconds
- Invalid date formats to verify error handling
Example additions:
private static Stream<Arguments> provideValues() { return Stream.of( Arguments.of("2018-01-01", true, ZonedDateTime.of(2018, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC)), Arguments.of("2020-06-23Z", false, ZonedDateTime.of(2020, 6, 23, 0, 0, 0, 0, ZoneOffset.UTC)), Arguments.of("2020-06-23-04:00", false, ZonedDateTime.of(2020, 6, 23, 0, 0, 0, 0, ZoneOffset.of("-04:00"))), Arguments.of("2020-01-01", true, ZonedDateTime.of(2020, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC)), - Arguments.of("2018-01-01", true, ZonedDateTime.of(2018, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC))); + Arguments.of("2018-01-01", true, ZonedDateTime.of(2018, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC)), + Arguments.of("2020-06-23T15:30:45Z", false, ZonedDateTime.of(2020, 6, 23, 15, 30, 45, 0, ZoneOffset.UTC)), + Arguments.of("2020-06-23T15:30:45.123Z", false, ZonedDateTime.of(2020, 6, 23, 15, 30, 45, 123_000_000, ZoneOffset.UTC)));core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/Metapath10ParserBase.java (2)
11-13: Enhance class-level documentationThe current documentation is quite minimal. Consider expanding it to include:
- The role of this class in Metapath processing
- Brief explanation of ANTLR and its purpose
- Guidelines for extending this base class
- Relationship with the generated ANTLR parser
/** - * The base class for Metapath ANTLR processing. + * Base class for Metapath ANTLR-based parsing operations. This class extends the ANTLR Parser + * to provide specialized processing for Metapath expressions. + * + * <p>ANTLR (ANother Tool for Language Recognition) is used here to generate the parser + * for Metapath expressions. This base class provides common functionality that can be + * used by the generated parser implementation. + * + * <p>Implementations should extend this class to define specific parsing rules and + * behaviors for Metapath processing. */
33-48: Consider optimizing the keyword checking logicThe logical restructuring from negated OR conditions to AND conditions improves readability. However, there's room for optimization.
Consider these improvements:
- Cache the lookahead token to avoid repeated calls
- Use a set-based approach for better maintainability
protected boolean isFuncCall() { + int nextToken = getInputStream().LA(1); + return !KEYWORD_TOKENS.contains(nextToken); } + private static final Set<Integer> KEYWORD_TOKENS = Set.of( + Metapath10.KW_ARRAY, + Metapath10.KW_FLAG, + Metapath10.KW_COMMENT, + Metapath10.KW_DOCUMENT_NODE, + Metapath10.KW_ELEMENT, + Metapath10.KW_EMPTY_SEQUENCE, + Metapath10.KW_FUNCTION, + Metapath10.KW_IF, + Metapath10.KW_ITEM, + Metapath10.KW_MAP, + Metapath10.KW_NAMESPACE_NODE, + Metapath10.KW_NODE, + Metapath10.KW_PROCESSING_INSTRUCTION, + Metapath10.KW_SCHEMA_ATTRIBUTE, + Metapath10.KW_SCHEMA_ELEMENT, + Metapath10.KW_TEXT + );This approach:
- Reduces repeated method calls
- Makes it easier to maintain the keyword list
- Potentially improves performance with set-based lookup
- Makes the logic more concise and clearer
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractCommandExecutor.java (2)
15-18: Enhance class-level documentation for better clarity.Consider expanding the JavaDoc to better describe the class's role as an abstract base implementation and its relationship with
ICommandExecutor./** - * A base class for implementation that perform the operation supported by a - * command. + * An abstract base class that implements the {@link ICommandExecutor} interface, + * providing common functionality for command execution implementations. Concrete + * subclasses must implement the {@link #execute()} method to define specific + * command behavior. */
40-44: Enhance getter method documentation.While the documentation is present, consider adding more details about:
- The relationship between these components
- The non-null guarantee for
getCommand()- Usage guidelines for subclasses
/** - * Get the context of the command execution. + * Get the context of the command execution, which provides access to the + * execution environment and configuration settings needed for command processing. * * @return the context */ /** - * Get the parsed command line details. + * Get the parsed command line details containing the command options and + * arguments provided by the user during execution. * * @return the cli details */ /** - * Get the command associated with this execution. + * Get the command associated with this execution. This method is guaranteed to + * return a non-null value as it's validated during construction. * * @return the command + * @throws NullPointerException if the target command is not set in the calling context */Also applies to: 50-54, 63-67
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/object/AbstractAmbiguousTemporal.java (2)
15-17: Consider enhancing the documentation with examples and type parameter details.While the documentation clearly explains the purpose and implementation, it would be more helpful with:
- Usage examples showing how ambiguous temporal values are handled
- More detailed explanation of the
TYPEparameter and its constraintsAdd the following to the class documentation:
* Implementations of this class represent a temporal value which may not have a * timezone making it ambiguous as a point/window in time. * <p> + * For example: + * <pre> + * // Date without timezone: "2023-11-20T10:30:00" + * // Date with timezone: "2023-11-20T10:30:00-05:00" + * </pre> * <p> * @param <TYPE> -* the bound object type +* the bound object type that extends this class, used for proper type +* inheritance in implementing classes like {@code AmbiguousDate} or +* {@code AmbiguousDateTime}
Line range hint
28-52: Consider implementing value semantics methods.The class implementation is solid, but as it represents a value type, it should implement:
equals()andhashCode()for proper value comparisontoString()for debugging supportAdd the following methods:
@Override public boolean equals(Object obj) { if (this == obj) { return true; } if (!(obj instanceof AbstractAmbiguousTemporal)) { return false; } AbstractAmbiguousTemporal<?> other = (AbstractAmbiguousTemporal<?>) obj; return timeZone == other.timeZone && getValue().equals(other.getValue()); } @Override public int hashCode() { return Objects.hash(getValue(), timeZone); } @Override public String toString() { return String.format("%s[value=%s, hasTimeZone=%b]", getClass().getSimpleName(), getValue(), timeZone); }cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/ExitCode.java (2)
Line range hint
13-45: Consider using standard Unix exit code ranges.While the current sequential numbering (0-7) is clean, consider aligning with standard Unix exit code ranges for better interoperability:
- 0: Success (current OK)
- 1-2: Miscellaneous errors (current FAIL and IO_ERROR)
- 64-78: Command-line usage errors (EX_USAGE=64 for INVALID_COMMAND, etc.)
- 70: Internal software error (EX_SOFTWARE=70 for RUNTIME_ERROR)
This would make the CLI behavior more predictable for Unix users and scripts.
Line range hint
82-89: Add null-safety check for varargs parameter.The varargs parameter could contain null elements. Consider adding a null check to prevent NullPointerException:
@NonNull public ExitStatus exitMessage(@NonNull Object... messageArguments) { + for (Object arg : messageArguments) { + if (arg == null) { + throw new IllegalArgumentException("Message arguments cannot contain null values"); + } + } return new MessageExitStatus(this, messageArguments); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPAddressItem.java (1)
53-55: Consider logging the caught exceptionThe caught
IllegalStateExceptionis silently ignored. Consider logging it at debug level for better troubleshooting capabilities.cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommandExecutor.java (3)
14-16: Minor grammar fix needed in interface documentationThe Javadoc has a small grammatical error.
-* Implementing class perform the operation supported by a command. +* Implementing classes perform the operation supported by a command.
52-54: Update method documentation to match its purposeThe Javadoc description incorrectly describes this method as creating a new command executor, but it actually executes a command operation.
- * Create a new command executor. + * Execute a command operation.
Line range hint
1-65: Excellent command pattern implementationThe design effectively separates command execution concerns using functional programming concepts. This architecture:
- Promotes loose coupling through functional interfaces
- Allows for flexible command execution implementations
- Makes testing easier through function composition
- Ensures null-safety through annotations
Consider documenting these design decisions in the project's architecture documentation to help future contributors understand the pattern.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/ITokenItem.java (1)
53-60: Consider adding explicit null checks for enhanced robustness.While the method is annotated with
@NonNull, consider adding explicit null checks for the intermediate results to prevent potential NPEs:static ITokenItem cast(@NonNull IAnyAtomicItem item) { try { return item instanceof ITokenItem ? (ITokenItem) item - : valueOf(item.asString()); + : valueOf(java.util.Objects.requireNonNull(item.asString(), "String value cannot be null")); } catch (InvalidTypeMetapathException ex) { throw new InvalidValueForCastFunctionException(ex); } }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INcNameItem.java (1)
15-19: Enhance deprecation notice with migration guidanceWhile the interface is properly marked as deprecated with removal timeline, it would be helpful to provide guidance on the recommended migration path for consumers of this interface.
Add migration guidance to the Javadoc:
/** * An atomic Metapath item containing a non-colonized name (NCName) data value. + * + * @deprecated Since 0.7.0, will be removed in a future version. + * Use {@link RecommendedAlternative} instead. */ @Deprecated(forRemoval = true, since = "0.7.0")cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractTerminalCommand.java (4)
19-21: Consider enhancing class documentation.While the documentation is clear, it would be helpful to add more context about this class's role in the command processing hierarchy and mention any important responsibilities of terminal commands.
/** - * A base class for commands that have no children. + * A base class for terminal commands in the command processing hierarchy. + * Terminal commands represent leaf nodes that perform actual operations + * and cannot have child commands. */
25-33: Enhance method documentation with thread-safety details.The implementation is correct, but the documentation should mention thread-safety guarantees provided by the lazy initialization pattern.
/** * A utility method that can be used to get the current working directory. + * This method is thread-safe due to lazy initialization. * * @return the current working directory */
Line range hint
35-76: Consider enhancing input validation and error handling.The implementations of
resolveAgainstCWDmethods are solid, but could benefit from the following improvements:
- Add validation for relative/absolute paths and URIs
- Enhance error handling for edge cases
- Document normalization behavior more explicitly
Example improvement for the Path version:
@NonNull protected static Path resolveAgainstCWD(@NonNull Path path) { + if (path.isAbsolute()) { + throw new IllegalArgumentException("Path must be relative: " + path); + } return ObjectUtils.notNull(getCurrentWorkingDirectory().resolve(path).normalize()); }Documentation improvement example:
/** * A utility method that can be used to resolve a path against the current * working directory. * + * The path is normalized after resolution to remove any redundant name + * elements (like "." or ".."). Only relative paths are accepted. * * @param path * the path to resolve * * @return the resolved path + * @throws IllegalArgumentException if the path is absolute */
Unit tests needed for path resolution utilities
The methods
getCurrentWorkingDirectory()andresolveAgainstCWD()are widely used across multiple CLI commands for critical path and URI resolution functionality. While the code is well-documented, the lack of unit tests for these core utilities poses a reliability risk.Please add unit tests to
cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/covering:
- Path resolution with relative and absolute paths
- URI resolution with different URI schemes
- Edge cases like empty paths and malformed URIs
- Thread-safety of the lazy initialization
You can follow the testing patterns shown in the existing
ExitCodeTest.javawhich demonstrates proper test organization and assertion usage.🔗 Analysis chain
Line range hint
19-76: Add unit tests for path and URI resolution.Please ensure that unit tests are added to verify:
- Path resolution behavior for different scenarios
- URI resolution edge cases
- Error handling for invalid inputs
- Thread-safety of the lazy initialization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing test files fd -e java "AbstractTerminalCommandTest" "src/test" # Look for similar test patterns in other test files rg -l "resolveAgainstCWD|getCurrentWorkingDirectory" "src/test"Length of output: 258
Script:
#!/bin/bash # First, let's find the correct test directory structure fd -t d "test" # Then, let's check for any existing test files related to CLI processor fd -e java . | grep -i "test.*cli.*processor" # Look for similar test patterns in any test files across the codebase rg -l "resolveAgainstCWD|getCurrentWorkingDirectory" .Length of output: 1740
Script:
#!/bin/bash # Let's examine the existing test file in cli-processor to understand the testing patterns cat cli-processor/src/test/java/gov/nist/secauto/metaschema/cli/processor/ExitCodeTest.java # Let's also check how these methods are used in the codebase for better context rg -A 5 "resolveAgainstCWD|getCurrentWorkingDirectory" .Length of output: 13341
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/MarkupXmlStreamWriter.java (3)
Line range hint
6-16: Good move to impl package for better encapsulation!Moving this class to the
implpackage better reflects its role as an implementation detail. This change follows the best practice of hiding implementation classes from API consumers.Consider adding a package-info.java file to document the purpose of this implementation package and its relationship with the parent package.
18-21: Enhance class documentation with more detailsWhile the added Javadoc is good, it could be more comprehensive by:
- Explaining the relationship with AbstractMarkupWriter
- Documenting thread-safety guarantees
- Providing usage examples
Consider expanding the class documentation like this:
/** * Provides support for writing Flexmark markup nodes to an XML stream. * This implementation uses {@link XMLStreamWriter} to write markup elements * and their attributes in a streaming fashion. * * <p>This class is not thread-safe and should not be shared between threads * without external synchronization. * * <p>Example usage: * <pre>{@code * XMLStreamWriter writer = ...; * MarkupXmlStreamWriter markupWriter = new MarkupXmlStreamWriter( * "http://example.com/ns", * ListOptions.getDefaultListOptions(), * writer); * }</pre> */
Line range hint
58-77: Add defensive programming checks for text contentThe text handling methods could benefit from additional null checks and validation:
Consider applying these improvements:
@Override public void writeText(CharSequence text) throws XMLStreamException { + if (text == null) { + return; + } getStream().writeCharacters(text.toString()); } @Override protected void writeComment(CharSequence text) throws XMLStreamException { + if (text == null) { + throw new IllegalArgumentException("Comment text cannot be null"); + } + if (text.toString().contains("--")) { + throw new IllegalArgumentException("XML comments cannot contain double-hyphens"); + } getStream().writeComment(text.toString()); }cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractParentCommand.java (3)
23-25: Enhance class documentationWhile the documentation is clear, consider adding:
- More details about the purpose and usage of this class
@sincetag to track API versioning/** - * A base class for a command that has child commands. + * A base class for a command that supports hierarchical command structure with child commands. + * This class provides the foundation for implementing complex CLI commands that can have + * multiple levels of subcommands. + * + * @since 1.0.0 */
30-34: Consider documenting thread safety guaranteesThe use of
Collections.synchronizedMapsuggests thread safety concerns. Consider documenting the thread safety guarantees this class provides in the class-level Javadoc./** * A base class for a command that has child commands. + * <p> + * This class is thread-safe and supports concurrent access to command handlers. + * </p> */
Line range hint
74-81: Improve error message constructionConsider extracting the sub-commands list construction to a separate method for better readability and reusability.
+ @NonNull + private String getSubCommandsList() { + return getSubCommands().stream() + .map(ICommand::getName) + .collect(Collectors.joining(", ")); + } + private ExitStatus executeCommand(...) { callingContext.showHelp(); ExitStatus status; if (isSubCommandRequired()) { status = ExitCode.INVALID_COMMAND - .exitMessage("Please use one of the following sub-commands: " + - getSubCommands().stream() - .map(ICommand::getName) - .collect(Collectors.joining(", "))); + .exitMessage("Please use one of the following sub-commands: " + getSubCommandsList()); } else { status = ExitCode.OK.exit(); } return status; }cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/AbstractExitStatus.java (3)
16-18: Enhance class documentation.While the documentation provides a basic description, consider enhancing it with:
- Implementation requirements for subclasses
- Description of the abstract contract
- @SInCE tag for version tracking
- Example usage pattern
/** * Records information about the exit status of a CLI command. + * + * <p>This abstract class provides base functionality for handling CLI command exit statuses, + * including error logging and throwable management. Implementing classes must provide + * the {@link #getMessage()} implementation to define the status message content. + * + * @since 1.0.0 */
Line range hint
67-78: Add Javadoc for getLogBuilder method.The method implementation looks good, but it's missing documentation to explain its purpose and return value.
+ /** + * Determines the appropriate LogBuilder based on the exit code status. + * For non-positive exit codes (success/info), returns an INFO level builder. + * For positive exit codes (errors), returns an ERROR level builder. + * + * @return the appropriate LogBuilder based on exit status, or null if logging is disabled + * at the determined level + */ @Nullable private LogBuilder getLogBuilder() {
80-101: Improve method documentation and code style.The implementation looks good, but consider these improvements:
- Add detailed Javadoc explaining the method's behavior
- Remove redundant comment and empty line
+ /** + * Generates and logs a message based on the current exit status. + * The message is logged at either INFO level (for success/info status) + * or ERROR level (for error status). + * + * @param showStackTrace if true and a throwable is present, includes the stack trace in the log + */ @Override public void generateMessage(boolean showStackTrace) { - // ... rest of the implementation ... - - // log the throwable + // ... rest of the implementation without empty line and redundant comment ...core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/MapEntry.java (1)
32-32: LGTM! Good encapsulation improvement.The change from public to package-private visibility for the
SIGNATUREconstant improves encapsulation and follows the principle of least privilege. This is a good practice as the signature should only be accessed within the function library package.Consider adding a brief Javadoc comment for the
SIGNATUREconstant to document its purpose and package-private scope:+ /** Function signature for the map:entry function. Package-private for use within the function library. */ static final IFunction SIGNATURE = IFunction.builder()core/src/test/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateTimeAdapterTest.java (2)
27-64: Consider enhancing test data organization and documentation.While the test cases are comprehensive, consider these improvements for better maintainability:
- Add Javadoc explaining the test data structure
- Group similar test cases with comments (e.g., "Cases with timezone", "Cases without timezone")
- Consider adding edge cases like leap years or invalid dates
+ /** + * Provides test cases for date-time parsing. + * Each argument contains: + * - input string to parse + * - boolean indicating if the datetime is ambiguous (no timezone) + * - expected ZonedDateTime result + * + * @return Stream of test cases + */ private static Stream<Arguments> provideValues() { return Stream.of( + // Cases without timezone (ambiguous) Arguments.of( "2018-01-01T00:00:00", true, ZonedDateTime.of(2018, 1, 1, 0, 0, 0, 0, ZoneOffset.UTC)), + // Cases with explicit timezone Arguments.of( "2019-09-28T23:20:50.52Z", false, ZonedDateTime.of(2019, 9, 28, 23, 20, 50, toNanos(0.52), ZoneOffset.UTC)),
67-69: Add documentation and input validation to the utility method.The nanosecond conversion logic is correct, but could be more robust with documentation and validation.
+ /** + * Converts a fractional second to nanoseconds. + * + * @param fraction the fractional part of a second (0.0 to 0.999...) + * @return the equivalent nanoseconds + * @throws IllegalArgumentException if fraction is negative or >= 1 + */ private static int toNanos(double fraction) { + if (fraction < 0.0 || fraction >= 1.0) { + throw new IllegalArgumentException("Fraction must be between 0.0 and 0.999..."); + } return (int) Math.round(TimeUnit.SECONDS.toNanos(1) * fraction); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUriReferenceItem.java (1)
35-47: Consider enhancing error message formatting.The implementation is solid with proper error handling and type validation. However, the error message could be more readable by using a template literal style format.
Consider this minor improvement:
- String.format("Invalid URI reference value '%s'. %s", - value, - ex.getLocalizedMessage()), + String.format("Invalid URI reference value '%s': %s", + value, + ex.getLocalizedMessage()),core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IStringItem.java (1)
28-37: Consider enhancing error handling documentation and clarity.While the implementation is solid, consider these improvements:
- Document the validation rules that MetaschemaDataTypeProvider.STRING applies
- Consider providing context for the
nullparameter in theInvalidTypeMetapathExceptionconstructorAdd Javadoc to explain the validation:
/** * Construct a new item using the provided string {@code value}. * * @param value * a string value that must conform to Metaschema string validation rules * @return the new item * @throws InvalidTypeMetapathException if the value fails string validation */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/MetapathConstants.java (2)
59-64: Consider clarifying the relationship between function namespaces.The documentation could be enhanced to explain why NS_METAPATH_FUNCTIONS_EXTENDED is an alias of NS_METAPATH_FUNCTIONS and when developers should use one over the other.
/** - * The namespace URI for Metapath extension built-in functions. + * The namespace URI for Metapath extension built-in functions. This is currently + * an alias for {@link #NS_METAPATH_FUNCTIONS} and can be used when implementing + * custom extension functions to distinguish them from core functions. */
65-89: Consider standardizing the cross-reference format in prefix documentation.While the documentation is clear, consider adding @see tags to all prefix constants to maintain consistency with the URI constant documentation style.
/** * The namespace prefix for Metapath data types and built-in casting functions. + * + * @see #NS_METAPATH for the corresponding namespace URI */core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateTimeAdapter.java (1)
55-69: Consider enhancing error messages for better debuggingThe refactored parse method has good separation of concerns and proper exception handling. However, the error message could be more descriptive.
Consider this enhancement:
- IllegalArgumentException newEx = new IllegalArgumentException(ex2.getLocalizedMessage(), ex2); + IllegalArgumentException newEx = new IllegalArgumentException( + String.format("Failed to parse datetime value '%s'. Expected format: YYYY-MM-DDThh:mm:ss[.SSS](Z|[+-]hh:mm)", value), + ex2);core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUuidItem.java (2)
21-42: Consider using StringBuilder for error message construction.The error message construction using String.format could be more efficient, especially since it concatenates multiple strings and includes exception details.
Consider this alternative implementation:
- throw new InvalidTypeMetapathException( - null, - String.format("Invalid UUID value '%s'. %s", - value, - ex.getLocalizedMessage()), - ex); + StringBuilder message = new StringBuilder(64) + .append("Invalid UUID value '") + .append(value) + .append("'. ") + .append(ex.getLocalizedMessage()); + throw new InvalidTypeMetapathException(null, message.toString(), ex);
79-86: Enhance error handling documentation.While the implementation is correct, the comment about IllegalStateException could be more descriptive.
Consider updating the comment to better explain when IllegalStateException might occur:
- // asString can throw IllegalStateException exception + // IllegalStateException may be thrown by asString() if the item's value is in an invalid state + // InvalidTypeMetapathException may be thrown if the string representation is not a valid UUIDcore/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IMarkupItem.java (1)
19-21: LGTM: Clear and concise documentation.Consider adding
@sincetag to indicate when this interface was introduced or last modified significantly.core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBase64BinaryItem.java (1)
18-20: Consider enhancing interface documentation.While the current documentation is clear, it could be more comprehensive by including:
- The interface's role in the Metapath type system
- Common usage patterns
- Relationship with Base64BinaryItemImpl
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/FlexmarkFactory.java (1)
Line range hint
26-36: Consider enhancing the singleton pattern implementation.While the current implementation is thread-safe, consider these improvements:
- Use double-checked locking or an enum-based singleton for better performance
- Consider using dependency injection instead of a singleton for better testability
- The class currently handles multiple responsibilities (parsing, rendering, formatting, converting) - consider splitting these into separate services for better maintainability
Example of an enum-based singleton pattern:
public enum FlexmarkFactory { INSTANCE; private final Parser markdownParser; private final HtmlRenderer htmlRenderer; // ... other fields ... FlexmarkFactory() { // initialization code } public static FlexmarkFactory getInstance() { return INSTANCE; } }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/CastFunction.java (1)
Line range hint
41-41: Fix typo in parameter nameThe parameter name
resulingAtomicTypecontains a typo and should beresultingAtomicType.- @NonNull Class<ITEM> resulingAtomicType, + @NonNull Class<ITEM> resultingAtomicType,core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDateItem.java (1)
21-23: Consider enhancing the interface documentation.While the current Javadoc provides a basic description, it would be beneficial to include more details about:
- The purpose and role of this interface in the Metapath system
- The relationship with AmbiguousDate and how it handles date values
- Any key behaviors or constraints that implementers should be aware of
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDateTimeItem.java (1)
19-21: Consider enhancing the interface documentation.While the current documentation is clear, it could be more descriptive by including:
- The purpose of this interface in the Metapath context
- Key capabilities and features
- Relationship with time zones
Example enhancement:
/** - * An atomic Metapath item containing a date/time data value. + * An atomic Metapath item representing a date/time value in the Metapath system. + * <p> + * This interface provides functionality for handling date/time values with and without + * time zone information, supporting parsing, casting, and comparison operations. + * It works in conjunction with {@link AmbiguousDateTime} to properly handle + * time zone ambiguity. */core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/HtmlCodeRenderExtension.java (1)
35-37: Enhance class documentationWhile the added documentation is a good start, consider expanding it to provide more context about:
- The purpose of this extension in the broader Flexmark/Metaschema context
- How it differs from standard code block handling
- Any specific HTML rendering behaviors it implements
/** - * Ensures code blocks are properly handled. + * A Flexmark HTML renderer extension that customizes the rendering of code blocks + * within Metaschema documents. This implementation provides special handling for + * source position tracking and custom HTML tags while maintaining standard + * Markdown code block semantics. */core/src/test/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/MarkupParserTest.java (2)
Line range hint
31-69: Enhance test coverage with specific assertions.While the test verifies basic parsing functionality, consider these improvements:
- Add assertions to verify the parsed content structure
- Include negative test cases (malformed XML, invalid markup)
- Uncomment and implement the debug statements or remove them
- Add assertions for specific HTML elements (tables, lists, etc.)
Example enhancement:
assertDoesNotThrow(() -> { MarkupMultiline markupString = XmlMarkupParser.instance().parseMarkupMultiline(reader, resource); - AstCollectingVisitor.asString(markupString.getDocument()); - // System.out.println(html); - // System.out.println(visitor.getAst()); - // System.out.println(markupString.toMarkdown()); + String ast = AstCollectingVisitor.asString(markupString.getDocument()); + + // Verify specific elements are properly parsed + assertTrue(ast.contains("list item"), "List items should be present in AST"); + assertTrue(ast.contains("table"), "Table should be present in AST"); + + // Verify markup conversion + String markdown = markupString.toMarkdown(); + assertTrue(markdown.contains("**list item**"), "Strong text should be converted to markdown"); });
Line range hint
71-98: Improve empty paragraph test validation.The test could be enhanced to better verify empty paragraph handling:
- Add specific assertions to verify the output
- Consider renaming to better describe the test purpose (e.g.,
testEmptyParagraphHandling)- Add test cases for multiple consecutive empty paragraphs
Example enhancement:
-void emptyParagraphTest() throws XMLStreamException { +void testEmptyParagraphHandling() throws XMLStreamException { // ... existing setup ... assertDoesNotThrow(() -> { MarkupMultiline ms = XmlMarkupParser.instance().parseMarkupMultiline(reader, resource); - LOGGER.atDebug().log("AST: {}", AstCollectingVisitor.asString(ms.getDocument())); - LOGGER.atDebug().log("HTML: {}", ms.toXHtml("")); - LOGGER.atDebug().log("Markdown: {}", ms.toMarkdown()); + String ast = AstCollectingVisitor.asString(ms.getDocument()); + String html = ms.toXHtml(""); + String markdown = ms.toMarkdown(); + + // Verify empty paragraph handling + assertTrue(ast.contains("paragraph"), "Empty paragraph should be present in AST"); + assertTrue(html.contains("<p></p>"), "Empty paragraph should be preserved in HTML"); + assertTrue(markdown.trim().isEmpty(), "Empty paragraph should produce empty markdown"); }); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INumericItem.java (2)
21-23: Consider enhancing the interface documentation.While the current documentation is accurate, it could be more descriptive by including:
- The purpose and role of numeric items in Metapath
- Common use cases or examples
- Relationship with other atomic item types
/** - * An atomic Metapath item containing a numeric data value. + * Represents an atomic Metapath item containing a numeric data value, which can be + * either an integer or decimal. This interface provides operations for numeric + * type conversion, comparison, and mathematical operations commonly used in + * Metapath expressions. + * + * @see IIntegerItem + * @see IDecimalItem */
137-164: Consider improvements to the rounding helper methods.While the implementation is functionally correct, consider the following improvements:
- The methods could be restructured to avoid the need for @SuppressWarnings("PMD.OnlyOneReturn")
- The mathematical operations, especially in roundWithNegativePrecision, would benefit from documentation explaining the algorithm
- Consider replacing the magic number '0' with a named constant
Here's a suggested refactoring:
+ private static final int ZERO = 0; + @NonNull - @SuppressWarnings("PMD.OnlyOneReturn") private INumericItem roundWithPositivePrecision(int precision) { + INumericItem result; if (this instanceof IIntegerItem) { - return this; + result = this; + } else { + BigDecimal value = asDecimal(); + BigDecimal rounded = value.signum() == -1 + ? value.round(new MathContext(precision + value.precision() - value.scale(), RoundingMode.HALF_DOWN)) + : value.round(new MathContext(precision + value.precision() - value.scale(), RoundingMode.HALF_UP)); + result = castAsType(IDecimalItem.valueOf(ObjectUtils.notNull(rounded))); } - BigDecimal value = asDecimal(); - BigDecimal rounded = value.signum() == -1 - ? value.round(new MathContext(precision + value.precision() - value.scale(), RoundingMode.HALF_DOWN)) - : value.round(new MathContext(precision + value.precision() - value.scale(), RoundingMode.HALF_UP)); - return castAsType(IDecimalItem.valueOf(ObjectUtils.notNull(rounded))); + return result; } + /** + * Rounds a number to the specified negative precision by: + * 1. Computing the divisor (10^|precision|) + * 2. If the absolute value is less than the divisor, returns 0 + * 3. Otherwise, rounds to the nearest multiple of the divisor + * + * @param precision the negative precision to round to + * @return the rounded value + */ @NonNull - @SuppressWarnings("PMD.OnlyOneReturn") private INumericItem roundWithNegativePrecision(int precision) { + INumericItem result; BigInteger value = asInteger(); - BigInteger divisor = BigInteger.TEN.pow(0 - precision); + BigInteger divisor = BigInteger.TEN.pow(ZERO - precision); if (divisor.compareTo(value.abs()) > 0) { - return IIntegerItem.ZERO; + result = IIntegerItem.ZERO; + } else { + BigInteger remainder = value.mod(divisor); + BigInteger lessRemainder = value.subtract(remainder); + BigInteger halfDivisor = divisor.divide(BigInteger.TWO); + BigInteger roundedValue = remainder.compareTo(halfDivisor) >= 0 + ? lessRemainder.add(divisor) + : lessRemainder; + result = IIntegerItem.valueOf(ObjectUtils.notNull(roundedValue)); } - BigInteger remainder = value.mod(divisor); - BigInteger lessRemainder = value.subtract(remainder); - BigInteger halfDivisor = divisor.divide(BigInteger.TWO); - BigInteger result = remainder.compareTo(halfDivisor) >= 0 - ? lessRemainder.add(divisor) - : lessRemainder; - return IIntegerItem.valueOf(ObjectUtils.notNull(result)); + return result; }cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommand.java (2)
46-54: Consider enhancing the documentation.While the implementation is correct, consider adding
@sincetag to document when this method was introduced, as it appears to be a new addition.
85-89: Consider adding parameter validation.While the implementation is correct, consider adding a null check for the
nameparameter to fail fast with a meaningful exception.@Nullable default ICommand getSubCommandByName(@NonNull String name) { + Objects.requireNonNull(name, "name must not be null"); // no sub-commands by default return null; }metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/GenerateSchemaCommand.java (1)
Line range hint
102-157: Consider breaking down the executeCommand method for better maintainability.While the method is well-structured, its complexity could be reduced by extracting some logic into helper methods:
- Destination path handling
- Configuration setup for inline types
- Schema generation logic
Here's a suggested approach:
protected void executeCommand( @NonNull CallingContext callingContext, @NonNull CommandLine cmdLine) throws CommandExecutionException { List<String> extraArgs = cmdLine.getArgList(); - Path destination = extraArgs.size() > 1 - ? MetaschemaCommands.handleDestination( - ObjectUtils.requireNonNull(extraArgs.get(1)), - cmdLine) - : null; + Path destination = resolveDestinationPath(extraArgs, cmdLine); SchemaFormat asFormat = MetaschemaCommands.getSchemaFormat(cmdLine, MetaschemaCommands.AS_SCHEMA_FORMAT_OPTION); - IMutableConfiguration<SchemaGenerationFeature<?>> configuration = new DefaultConfiguration<>(); - if (cmdLine.hasOption(INLINE_TYPES_OPTION)) { - configuration.enableFeature(SchemaGenerationFeature.INLINE_DEFINITIONS); - if (SchemaFormat.JSON.equals(asFormat)) { - configuration.disableFeature(SchemaGenerationFeature.INLINE_CHOICE_DEFINITIONS); - } else { - configuration.enableFeature(SchemaGenerationFeature.INLINE_CHOICE_DEFINITIONS); - } - } + IMutableConfiguration<SchemaGenerationFeature<?>> configuration = createConfiguration(cmdLine, asFormat); + generateSchema(extraArgs, destination, asFormat, configuration); + } + + private Path resolveDestinationPath(List<String> extraArgs, CommandLine cmdLine) { + return extraArgs.size() > 1 + ? MetaschemaCommands.handleDestination( + ObjectUtils.requireNonNull(extraArgs.get(1)), + cmdLine) + : null; + } + + private IMutableConfiguration<SchemaGenerationFeature<?>> createConfiguration( + CommandLine cmdLine, SchemaFormat asFormat) { + IMutableConfiguration<SchemaGenerationFeature<?>> configuration = new DefaultConfiguration<>(); + if (cmdLine.hasOption(INLINE_TYPES_OPTION)) { + configuration.enableFeature(SchemaGenerationFeature.INLINE_DEFINITIONS); + if (SchemaFormat.JSON.equals(asFormat)) { + configuration.disableFeature(SchemaGenerationFeature.INLINE_CHOICE_DEFINITIONS); + } else { + configuration.enableFeature(SchemaGenerationFeature.INLINE_CHOICE_DEFINITIONS); + } + } + return configuration; + } + + private void generateSchema( + List<String> extraArgs, + Path destination, + SchemaFormat asFormat, + IMutableConfiguration<SchemaGenerationFeature<?>> configuration) throws CommandExecutionException {core/src/main/java/gov/nist/secauto/metaschema/core/model/xml/ModuleLoader.java (1)
61-65: Enhance constructor documentation.Consider enhancing the constructor's Javadoc to reference the new factory method for cases where constraints are needed. This helps guide users to the appropriate instantiation method.
Add a reference to the factory method in the Javadoc:
/** * Construct a new Metaschema loader with no constraints. * * @see #newInstanceUsingConstraints(Collection) for creating an instance with constraints */core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnMinMax.java (1)
Line range hint
191-217: Consider improving the createNormalizedStream methodWhile the logic is correct, there are several opportunities for improvement:
- The stream variable reassignment pattern could be replaced with a more functional approach
- The error message could be more readable
- The nested conditions could be simplified
Here's a suggested refactoring:
@NonNull private static Stream<? extends IAnyAtomicItem> createNormalizedStream( @NonNull List<? extends IAnyAtomicItem> items, @NonNull Map<Class<? extends IAnyAtomicItem>, Integer> counts) { - Stream<? extends IAnyAtomicItem> stream = null; - if (counts.size() == 1) { - stream = items.stream(); - } else if (counts.size() > 1) { - int size = items.size(); - if (counts.getOrDefault(IStringItem.class, 0) + counts.getOrDefault(IAnyUriItem.class, 0) == size) { - stream = items.stream() - .map(IAnyAtomicItem::asStringItem); - } else if (counts.getOrDefault(IDecimalItem.class, 0) == size) { - stream = items.stream() - .map(item -> (IDecimalItem) item); - } - } - - if (stream == null) { + int size = items.size(); + + // Single type - no conversion needed + if (counts.size() == 1) { + return items.stream(); + } + + // Multiple types - attempt conversion + if (counts.size() > 1) { + // Check if all items are either String or AnyUri + if (counts.getOrDefault(IStringItem.class, 0) + counts.getOrDefault(IAnyUriItem.class, 0) == size) { + return items.stream().map(IAnyAtomicItem::asStringItem); + } + + // Check if all items are Decimal + if (counts.getOrDefault(IDecimalItem.class, 0) == size) { + return items.stream().map(item -> (IDecimalItem) item); + } + } + + // No valid conversion possible throw new InvalidArgumentFunctionException( InvalidArgumentFunctionException.INVALID_ARGUMENT_TYPE, - String.format("Values must all be of a single atomic type. Their types are '%s'.", - FunctionUtils.getTypes(items).stream() - .map(Class::getName) - .collect(Collectors.joining(",")))); + String.format( + "Values must all be of a single atomic type. Found multiple types: [%s]", + FunctionUtils.getTypes(items).stream() + .map(Class::getSimpleName) + .collect(Collectors.joining(", ")))); - } - return stream; }The refactored version:
- Eliminates the need for a mutable stream variable
- Uses early returns for better readability
- Improves the error message format
- Uses Class::getSimpleName for more readable type names
- Adds comments explaining the logic
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/MetapathExpression.java (1)
Line range hint
50-77: LGTM! Well-structured enum with comprehensive documentation.The
ResultTypeenum effectively defines and documents the expected types for Metapath evaluation results, providing good type safety. The implementation aligns well with thetoResultTypemethod usage.Consider adding a method to get the corresponding Java class type for each enum constant, which could be useful for runtime type checking. Example:
public Class<?> getJavaType() { switch (this) { case NUMBER: return BigDecimal.class; case STRING: return String.class; case BOOLEAN: return Boolean.class; case SEQUENCE: return ISequence.class; case ITEM: return IItem.class; default: throw new IllegalStateException("Unexpected value: " + this); } }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/StaticContext.java (1)
377-381: Consider initializing all function namespaces in Builder constructorThe Builder constructor only initializes the core and math function namespaces, while WELL_KNOWN_NAMESPACES also includes array and map function namespaces. Consider adding these for consistency:
private Builder() { namespaces.put( MetapathConstants.PREFIX_METAPATH, MetapathConstants.NS_METAPATH); namespaces.put( MetapathConstants.PREFIX_METAPATH_FUNCTIONS, MetapathConstants.NS_METAPATH_FUNCTIONS); namespaces.put( MetapathConstants.PREFIX_METAPATH_FUNCTIONS_MATH, MetapathConstants.NS_METAPATH_FUNCTIONS_MATH); + namespaces.put( + MetapathConstants.PREFIX_METAPATH_FUNCTIONS_ARRAY, + MetapathConstants.NS_METAPATH_FUNCTIONS_ARRAY); + namespaces.put( + MetapathConstants.PREFIX_METAPATH_FUNCTIONS_MAP, + MetapathConstants.NS_METAPATH_FUNCTIONS_MAP); }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java (1)
37-37: Document the reason for suppressing deprecation warningsWhile the addition of
@SuppressWarnings("deprecation")aligns with addressing compile-time warnings, it would be helpful to document why this suppression is necessary, similar to how "// NOPMD - intentional" explains the PMD suppression.Add a comment explaining which deprecated APIs are being used and why they can't be replaced yet:
+ // TODO: Using deprecated API <name> because <reason>. Can be removed when <condition> @SuppressWarnings("deprecation")metaschema-testing/src/main/java/gov/nist/secauto/metaschema/model/testing/AbstractTestSuite.java (2)
60-69: Consider breaking down this class to address PMD warningsThe PMD suppressions for
GodClassandCouplingBetweenObjectsindicate that this class might be doing too much and has too many dependencies. Consider:
- Extracting schema generation logic into a separate class
- Creating a dedicated test case generator class
- Moving validation logic to a separate validator class
239-270: LGTM: Well-structured schema generation logicThe method is well-implemented with:
- Proper error handling
- Exhaustive format handling
- Appropriate null checks
Consider extracting the schema extension logic into a separate method or enum to improve maintainability.
+ private static String getSchemaExtension(Format format) { + switch (format) { + case JSON: + case YAML: + return ".json"; + case XML: + return ".xsd"; + default: + throw new IllegalStateException("Unsupported format: " + format); + } + }core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/AbstractAstVisitor.java (1)
Line range hint
1-1006: Excellent visitor pattern implementation with room for minor improvementThe class demonstrates excellent architectural decisions:
- Clear separation of concerns with abstract handle methods
- Consistent error handling patterns
- Comprehensive documentation with W3C spec references
- Proper null checking with assertions
Consider extracting the IllegalStateException messages into constants to maintain consistency and ease maintenance.
Example implementation:
private static final String ERR_DELEGATE_ONLY = "This method should never be called directly as it is handled by the parent expression"; private static final String ERR_SINGLE_CHILD = "A single child expression was expected"; // Then use these constants: throw new IllegalStateException(ERR_DELEGATE_ONLY);core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/HtmlQuoteTagExtension.java (1)
Line range hint
161-174: Avoid Suppressing Warnings for Constructor Calls to Overridable MethodsThe use of
@SuppressWarnings("PMD.ConstructorCallsOverridableMethod")indicates that overridable methods are being called within the constructor. This can lead to unexpected behavior if subclasses override these methods.Consider the following actions:
- Make the called methods
final: If possible, declaresetTypographicOpening()andsetTypographicClosing()asfinalin their respective classes to prevent them from being overridden.- Refactor to avoid calling overridable methods: Initialize the necessary fields directly within the constructor instead of using setter methods.
Applying these changes can enhance the robustness of the class and prevent potential issues in subclassing scenarios.
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/InsertAnchorExtension.java (1)
53-56: Improve class-level JavaDoc for clarity and standardizationConsider rephrasing the JavaDoc to adhere to standard conventions. This enhances readability and aligns with typical JavaDoc practices.
Apply this diff to update the JavaDoc:
/** - * This adds support for insert anchors, which are used in OSCAL statements - * currently, but can be used more generally in other OSCAL models. + * Extension that adds support for insert anchors, used in OSCAL statements, + * and applicable more generally in other OSCAL models. */cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessor.java (2)
63-66: Avoid redundant null checks on Option buildersThe
Option.builder()methods from Apache Commons CLI do not return null. Therefore, wrapping them withObjectUtils.notNull()is unnecessary and adds redundant code. You can safely remove these null checks to simplify the code and improve readability.Apply this refactoring to the option declarations:
- @NonNull - public static final Option HELP_OPTION = ObjectUtils.notNull(Option.builder("h") + @NonNull + public static final Option HELP_OPTION = Option.builder("h") .longOpt("help") .desc("display this help message") .build(); - @NonNull - public static final Option NO_COLOR_OPTION = ObjectUtils.notNull(Option.builder() + @NonNull + public static final Option NO_COLOR_OPTION = Option.builder() .longOpt("no-color") .desc("do not colorize output") .build(); - @NonNull - public static final Option QUIET_OPTION = ObjectUtils.notNull(Option.builder("q") + @NonNull + public static final Option QUIET_OPTION = Option.builder("q") .longOpt("quiet") .desc("minimize output to include only errors") .build(); - @NonNull - public static final Option SHOW_STACK_TRACE_OPTION = ObjectUtils.notNull(Option.builder() + @NonNull + public static final Option SHOW_STACK_TRACE_OPTION = Option.builder() .longOpt("show-stack-trace") .desc("display the stack trace associated with an error") .build(); - @NonNull - public static final Option VERSION_OPTION = ObjectUtils.notNull(Option.builder() + @NonNull + public static final Option VERSION_OPTION = Option.builder() .longOpt("version") .desc("display the application version") .build();Also applies to: 71-74, 79-82, 88-91, 96-99
306-306: Refactor 'CallingContext' class to reduce complexityThe
CallingContextclass is annotated with@SuppressWarnings("PMD.GodClass"), indicating that it may have too many responsibilities or is too complex. Consider refactoring this class by extracting some of its functionality into separate, cohesive classes or methods. This will adhere to the Single Responsibility Principle and enhance maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (113)
cli-processor/src/main/java-templates/gov/nist/secauto/metaschema/cli/processor/ProcessorVersion.java(1 hunks)cli-processor/src/main/java-templates/gov/nist/secauto/metaschema/cli/processor/Version.java(0 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/AbstractExitStatus.java(3 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessor.java(20 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/ExitCode.java(1 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/ExitStatus.java(2 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/InvalidArgumentException.java(1 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/MessageExitStatus.java(2 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/NonMessageExitStatus.java(1 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/OptionUtils.java(1 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/VersionInfo.java(0 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractCommandExecutor.java(2 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractParentCommand.java(4 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractTerminalCommand.java(1 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/CommandService.java(4 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/DefaultExtraArgument.java(0 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ExtraArgument.java(1 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommand.java(1 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommandExecutor.java(2 hunks)cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/impl/DefaultExtraArgument.java(1 hunks)core/src/main/java-templates/gov/nist/secauto/metaschema/core/MetaschemaJavaVersion.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/MetaschemaConstants.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/Base64Adapter.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapter.java(6 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateTimeAdapter.java(5 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/HostnameAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/UuidAdapter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/AbstractMarkupString.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupLine.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupLineAdapter.java(0 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupMultilineAdapter.java(0 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/XmlMarkupParser.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/FlexmarkFactory.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/HtmlQuoteTagExtension.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/InsertAnchorExtension.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/AbstractMarkupWriter.java(0 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/AstCollectingVisitor.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/FixedEmphasisDelimiterProcessor.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/FlexmarkConfiguration.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/HtmlCodeRenderExtension.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/IMarkupVisitor.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/IMarkupWriter.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/MarkupVisitor.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/MarkupXmlEventWriter.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/MarkupXmlStreamWriter.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/SuppressPTagExtension.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/object/AbstractAmbiguousTemporal.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/object/AmbiguousDate.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/datatype/object/AmbiguousDateTime.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/EQNameUtils.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/ICollectionValue.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/IPrintable.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/MetapathConstants.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/MetapathExpression.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/StaticContext.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/AbstractAstVisitor.java(26 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/FailingErrorListener.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/Metapath10ParserBase.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/ParseTreePrinter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/ComparisonFunctions.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/CastFunction.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnMinMax.java(4 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/MapEntry.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IAnyAtomicItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IAnyUriItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBase64BinaryItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBooleanItem.java(5 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDateItem.java(4 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDateTimeItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDayTimeDurationItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDecimalItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDurationItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IEmailAddressItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IHostnameItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPAddressItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv4AddressItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv6AddressItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIntegerItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IMarkupItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INcNameItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java(5 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INumericItem.java(4 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IPositiveIntegerItem.java(4 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IStringItem.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/ITemporalItem.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/ITokenItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUntypedAtomicItem.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUriReferenceItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUuidItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IYearMonthDurationItem.java(4 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractStringItem.java(2 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateTimeWithoutTimeZoneItemImpl.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateWithoutTimeZoneItemImpl.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/UuidItemImpl.java(3 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/model/xml/ModuleLoader.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/model/xml/impl/XmlbeansMarkupWriter.java(1 hunks)core/src/main/java/gov/nist/secauto/metaschema/core/package-info.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapterTest.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateTimeAdapterTest.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/datatype/markup/CommonmarkConformanceTest.java(0 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupStringTest.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/MarkupParserTest.java(1 hunks)core/src/test/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnCeilingTest.java(2 hunks)metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/AbstractConvertSubcommand.java(1 hunks)metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/AbstractValidateContentCommand.java(1 hunks)metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/GenerateDiagramCommand.java(1 hunks)metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/GenerateSchemaCommand.java(1 hunks)metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/metapath/MetapathCommand.java(0 hunks)metaschema-testing/src/main/java/gov/nist/secauto/metaschema/model/testing/AbstractTestSuite.java(5 hunks)metaschema-testing/src/main/java/gov/nist/secauto/metaschema/model/testing/xml/xmlbeans/handler/FormatType.java(1 hunks)metaschema-testing/src/main/java/gov/nist/secauto/metaschema/model/testing/xml/xmlbeans/handler/GenerationResultType.java(1 hunks)metaschema-testing/src/main/java/gov/nist/secauto/metaschema/model/testing/xml/xmlbeans/handler/ValidationResultType.java(1 hunks)
💤 Files with no reviewable changes (8)
- cli-processor/src/main/java-templates/gov/nist/secauto/metaschema/cli/processor/Version.java
- cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/VersionInfo.java
- cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/DefaultExtraArgument.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupLineAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupMultilineAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/AbstractMarkupWriter.java
- core/src/test/java/gov/nist/secauto/metaschema/core/datatype/markup/CommonmarkConformanceTest.java
- metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/metapath/MetapathCommand.java
✅ Files skipped from review due to trivial changes (17)
- cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/OptionUtils.java
- core/src/main/java/gov/nist/secauto/metaschema/core/MetaschemaConstants.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/XmlMarkupParser.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/IMarkupVisitor.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/IMarkupWriter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/MarkupVisitor.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/MarkupXmlEventWriter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/SuppressPTagExtension.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/EQNameUtils.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/ICollectionValue.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/IPrintable.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/ParseTreePrinter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/model/xml/impl/XmlbeansMarkupWriter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/package-info.java
- metaschema-testing/src/main/java/gov/nist/secauto/metaschema/model/testing/xml/xmlbeans/handler/FormatType.java
- metaschema-testing/src/main/java/gov/nist/secauto/metaschema/model/testing/xml/xmlbeans/handler/GenerationResultType.java
- metaschema-testing/src/main/java/gov/nist/secauto/metaschema/model/testing/xml/xmlbeans/handler/ValidationResultType.java
🚧 Files skipped from review as they are similar to previous changes (20)
- core/src/main/java-templates/gov/nist/secauto/metaschema/core/MetaschemaJavaVersion.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/Base64Adapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/HostnameAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/UuidAdapter.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/AbstractMarkupString.java
- core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupLine.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/ComparisonFunctions.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IAnyAtomicItem.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDayTimeDurationItem.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDecimalItem.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDurationItem.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IEmailAddressItem.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IHostnameItem.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/ITemporalItem.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUntypedAtomicItem.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IYearMonthDurationItem.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractStringItem.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateTimeWithoutTimeZoneItemImpl.java
- core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/UuidItemImpl.java
- core/src/test/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnCeilingTest.java
🧰 Additional context used
📓 Learnings (13)
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapter.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapter.java:41-48
Timestamp: 2024-11-14T23:37:29.087Z
Learning: In the `DateAdapter` class (`core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapter.java`), using a regular expression for date validation is necessary because `DateTimeFormatter` does not properly parse dates with timezones in this context.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBase64BinaryItem.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/Base64BinaryItemImpl.java:6-6
Timestamp: 2024-11-14T05:17:43.127Z
Learning: When verifying package moves, ensure that references to the old package path are accurately identified to avoid reporting false positives.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBooleanItem.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBooleanItem.java:86-104
Timestamp: 2024-11-14T18:19:40.200Z
Learning: In the file `core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBooleanItem.java`, the 3-step approach in the `cast` method is consistent with the XPath 3.1 specification.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDateTimeItem.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IYearMonthDurationItem.java:87-94
Timestamp: 2024-11-14T05:20:24.046Z
Learning: In the `IYearMonthDurationItem` interface (`core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IYearMonthDurationItem.java`), the `cast` method uses `item.asString()` as intended. Do not suggest replacing it with `FunctionUtils.toString(item)` in this context.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPAddressItem.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv4AddressItem.java:66-73
Timestamp: 2024-11-14T17:07:03.587Z
Learning: In the Metaschema Java codebase, differences in casting patterns across atomic type implementations are intentional and required; any differences in approach are significant and necessary.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv4AddressItem.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv4AddressItem.java:66-73
Timestamp: 2024-11-14T17:07:03.587Z
Learning: In the Metaschema Java codebase, differences in casting patterns across atomic type implementations are intentional and required; any differences in approach are significant and necessary.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv6AddressItem.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv4AddressItem.java:66-73
Timestamp: 2024-11-14T17:07:03.587Z
Learning: In the Metaschema Java codebase, differences in casting patterns across atomic type implementations are intentional and required; any differences in approach are significant and necessary.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIntegerItem.java (3)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java:116-124
Timestamp: 2024-11-14T17:09:05.819Z
Learning: In the interface `INonNegativeIntegerItem` (file `core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java`), the casting logic in the `cast` method is intentionally designed this way due to earlier discrepancies.
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/NonNegativeIntegerItemImpl.java:24-31
Timestamp: 2024-11-14T05:15:46.640Z
Learning: In the `NonNegativeIntegerItemImpl` class (`core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/NonNegativeIntegerItemImpl.java`), the validation for non-negative values is performed in the `INonNegativeIntegerItem.valueOf(BigInteger)` method. Therefore, it's not necessary to duplicate this validation in the constructor of `NonNegativeIntegerItemImpl`.
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java:66-67
Timestamp: 2024-11-14T17:08:15.228Z
Learning: In the `INonNegativeIntegerItem.valueOf(@NonNull INumericItem value)` method (file: `core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java`), the loss of precision when converting an `INumericItem` to an integer using `value.asInteger()` is intentional, as this method is meant for converting the provided numeric into an integer value.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java (3)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java:116-124
Timestamp: 2024-11-14T17:09:05.819Z
Learning: In the interface `INonNegativeIntegerItem` (file `core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java`), the casting logic in the `cast` method is intentionally designed this way due to earlier discrepancies.
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java:66-67
Timestamp: 2024-11-14T17:08:15.228Z
Learning: In the `INonNegativeIntegerItem.valueOf(@NonNull INumericItem value)` method (file: `core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java`), the loss of precision when converting an `INumericItem` to an integer using `value.asInteger()` is intentional, as this method is meant for converting the provided numeric into an integer value.
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/NonNegativeIntegerItemImpl.java:24-31
Timestamp: 2024-11-14T05:15:46.640Z
Learning: In the `NonNegativeIntegerItemImpl` class (`core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/NonNegativeIntegerItemImpl.java`), the validation for non-negative values is performed in the `INonNegativeIntegerItem.valueOf(BigInteger)` method. Therefore, it's not necessary to duplicate this validation in the constructor of `NonNegativeIntegerItemImpl`.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INumericItem.java (2)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java:116-124
Timestamp: 2024-11-14T17:09:05.819Z
Learning: In the interface `INonNegativeIntegerItem` (file `core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java`), the casting logic in the `cast` method is intentionally designed this way due to earlier discrepancies.
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java:66-67
Timestamp: 2024-11-14T17:08:15.228Z
Learning: In the `INonNegativeIntegerItem.valueOf(@NonNull INumericItem value)` method (file: `core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java`), the loss of precision when converting an `INumericItem` to an integer using `value.asInteger()` is intentional, as this method is meant for converting the provided numeric into an integer value.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IPositiveIntegerItem.java (3)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java:116-124
Timestamp: 2024-11-14T17:09:05.819Z
Learning: In the interface `INonNegativeIntegerItem` (file `core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java`), the casting logic in the `cast` method is intentionally designed this way due to earlier discrepancies.
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java:66-67
Timestamp: 2024-11-14T17:08:15.228Z
Learning: In the `INonNegativeIntegerItem.valueOf(@NonNull INumericItem value)` method (file: `core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java`), the loss of precision when converting an `INumericItem` to an integer using `value.asInteger()` is intentional, as this method is meant for converting the provided numeric into an integer value.
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/NonNegativeIntegerItemImpl.java:24-31
Timestamp: 2024-11-14T05:15:46.640Z
Learning: In the `NonNegativeIntegerItemImpl` class (`core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/NonNegativeIntegerItemImpl.java`), the validation for non-negative values is performed in the `INonNegativeIntegerItem.valueOf(BigInteger)` method. Therefore, it's not necessary to duplicate this validation in the constructor of `NonNegativeIntegerItemImpl`.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IStringItem.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IYearMonthDurationItem.java:87-94
Timestamp: 2024-11-14T05:20:24.046Z
Learning: In the `IYearMonthDurationItem` interface (`core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IYearMonthDurationItem.java`), the `cast` method uses `item.asString()` as intended. Do not suggest replacing it with `FunctionUtils.toString(item)` in this context.
core/src/test/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapterTest.java (1)
Learnt from: david-waltermire
PR: metaschema-framework/metaschema-java#245
File: core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapter.java:41-48
Timestamp: 2024-11-14T23:37:29.087Z
Learning: In the `DateAdapter` class (`core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapter.java`), using a regular expression for date validation is necessary because `DateTimeFormatter` does not properly parse dates with timezones in this context.
🔇 Additional comments (161)
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/NonMessageExitStatus.java (1)
Line range hint 13-24: Implementation looks good!
The class is well-structured, follows the single responsibility principle, and properly handles null safety. The suggested improvements are mainly documentation-related and don't affect the functionality.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/FailingErrorListener.java (1)
Line range hint 20-26: Implementation is clean and effective.
The error listener implementation is well-structured and provides clear error messages with location information, which is valuable for debugging syntax errors.
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/impl/DefaultExtraArgument.java (2)
16-17: LGTM! Well-designed immutable fields.
The fields follow good practices with clear naming and appropriate use of the final modifier for immutability.
32-40: LGTM! Clean and straightforward method implementations.
The getter methods are well-implemented with clear and concise code.
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/object/AmbiguousDate.java (3)
12-16: LGTM! Clear documentation and improved class naming.
The class renaming from Date to AmbiguousDate better reflects its purpose, and the documentation clearly explains the ambiguous nature of the date value with respect to timezone.
22-31: LGTM! Well-documented constructor with proper null safety.
The constructor is properly implemented with:
- Clear documentation explaining the ambiguity concept
- Proper null safety with @nonnull annotation
- Correct delegation to the superclass
16-16: Verify class rename impact across the codebase.
Let's ensure all references to the old Date class have been updated to use AmbiguousDate.
✅ Verification successful
Let me gather more specific information about potential references to the renamed class.
Let me modify the search patterns to work without look-around assertions.
Based on the search results, I can now provide the final response:
Class rename has been properly implemented across the codebase
The rename from Date to AmbiguousDate has been correctly implemented with all references properly updated. The search results show that:
- The new
AmbiguousDateclass is properly referenced in all relevant files - No lingering references to the old class name exist (excluding test class names and adapter classes which use "Date" as part of their descriptive names)
- All imports and type references have been updated to use
AmbiguousDate
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old class name that might have been missed
# Search for remaining references to the old class name
echo "Searching for potential missed references to 'Date'..."
rg --type java "class +Date" -g '!AmbiguousDate.java'
# Search for potential import statements that might need updating
echo "Checking for old import statements..."
rg --type java "import.*\.Date;" -g '!AmbiguousDate.java'
# Search for type references in method signatures and variable declarations
echo "Checking for type references..."
ast-grep --pattern 'Date $var;'
Length of output: 2150
Script:
#!/bin/bash
# Search for references to the old class name in method signatures, variable declarations, and instantiations
echo "Searching for references to the original Date class..."
rg --type java "(?<!Ambiguous)Date(?!\w)" -g '!AmbiguousDate.java' -A 2
# Search for specific imports of the old Date class
echo "Checking for specific imports of the Date class..."
rg --type java "import.*\.Date;" -g '!AmbiguousDate.java' --context 2
# Check for any references in XML or configuration files
echo "Checking configuration files..."
rg "(?<!Ambiguous)Date(?!\w)" -g '*.xml' -g '*.properties' -g '*.yaml' -g '*.yml'
Length of output: 990
Script:
#!/bin/bash
# Search for Date class references, excluding AmbiguousDate
echo "Searching for Date references in Java files..."
rg --type java "\bDate\b" -g '!AmbiguousDate.java' -g '!*Test.java' -g '!*Exception.java' --context 2
# Search for Date in type declarations and method signatures using ast-grep
echo "Checking type declarations and method signatures..."
ast-grep --pattern 'Date $var'
# Search for new class name to confirm the rename was applied
echo "Verifying AmbiguousDate usage..."
rg --type java "AmbiguousDate" --context 2
Length of output: 26083
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/object/AmbiguousDateTime.java (4)
12-16: LGTM! Clear documentation and improved naming.
The class renaming from DateTime to AmbiguousDateTime along with the added documentation clearly communicates the purpose of this class in handling date/time values that may lack timezone information. This change improves code clarity by making the ambiguous nature of the datetime explicit.
32-34: LGTM! Well-documented constructor with proper validation.
The constructor is well-implemented with comprehensive documentation, proper parameter validation using @NonNull, and correct delegation to the superclass.
37-39: LGTM! Proper implementation of the copy method.
The copy method correctly implements a deep copy by creating a new instance with the current values, maintaining immutability.
16-17: Verify the impact of the DateTime to AmbiguousDateTime rename.
The class design looks good with proper inheritance and clear responsibility. Let's verify that all references to the old DateTime class have been updated to use AmbiguousDateTime.
✅ Verification successful
Let me analyze the occurrences of "DateTime" to determine if they are legitimate uses or missed renames. I need to check the actual content of some key files.
Based on the analysis of the files, I can now generate the final response as the findings show legitimate uses of "DateTime" in different contexts.
The DateTime to AmbiguousDateTime rename has been properly implemented
The occurrences of "DateTime" found in the codebase are legitimate and fall into these categories:
- Proper usage of the new
AmbiguousDateTimeclass in adapters and implementations - Interface/class names like
IDateTimeItem,DateTimeWithTimeZoneItemImplwhich represent different concepts - References in date/time formatting and parsing utilities
The rename from DateTime to AmbiguousDateTime has been correctly implemented, with proper inheritance and all dependent code updated accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old DateTime class name
# that might have been missed during the rename.
echo "Searching for any remaining references to DateTime class..."
rg --type java "DateTime[^A-Za-z]" -l
echo "Checking import statements..."
rg --type java "import.*DateTime;" -l
echo "Checking Javadoc references..."
rg --type java "@.*DateTime[^A-Za-z]" -l
Length of output: 5922
Script:
#!/bin/bash
# Check content of key files to understand the DateTime references
echo "Checking AmbiguousDateTime usage..."
rg -A 2 -B 2 "AmbiguousDateTime" core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateTimeAdapter.java
echo "\nChecking DateTime references in IDateTimeItem..."
cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDateTimeItem.java
echo "\nChecking DateTime references in DateTimeWithTimeZoneItemImpl..."
cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateTimeWithTimeZoneItemImpl.java
Length of output: 7211
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/ExitStatus.java (1)
11-14: LGTM! Clear and concise interface documentation.
The added Javadoc clearly describes the interface's purpose and responsibility in the CLI processing context.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateWithoutTimeZoneItemImpl.java (4)
1-5: LGTM! Proper license headers.
The SPDX license headers are correctly formatted and clearly indicate the CC0-1.0 license.
6-15: LGTM! Clean and necessary imports.
The imports are specific, well-organized, and all are utilized in the implementation.
23-31: LGTM! Well-documented constructor with proper null checking.
The constructor is properly implemented with appropriate documentation and null-safety checks.
1-43: Verify integration with related date/time classes.
Let's ensure this implementation is consistently used across the codebase.
✅ Verification successful
Based on the shell script results, I can now provide a final response as the verification is complete:
Implementation is properly integrated within the date/time handling system
The codebase shows a well-structured and consistent approach to handling dates with and without timezones:
- Proper class hierarchy with distinct implementations:
DateWithoutTimeZoneItemImplDateWithTimeZoneItemImplDateTimeWithoutTimeZoneItemImplDateTimeWithTimeZoneItemImpl
- Consistent usage of
AmbiguousDateandAmbiguousDateTimeclasses for handling timezone ambiguity - No unsafe raw
Dateinstantiations found that might ignore timezone considerations - Complete integration with type adapters (
DateAdapter,DateTimeAdapter) for proper parsing and handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of DateWithoutTimeZoneItemImpl and related classes
# Find related date/time implementation classes
echo "Related implementation classes:"
fd -e java . | grep -i "date.*impl.java"
# Check for references to AmbiguousDate
echo -e "\nUsage of AmbiguousDate:"
rg "AmbiguousDate" -t java
# Look for potential date handling without proper timezone consideration
echo -e "\nPotential date handling without timezone consideration:"
ast-grep --pattern 'new Date($$$)' || true
Length of output: 7684
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ExtraArgument.java (4)
12-14: LGTM! Clear and concise documentation.
The interface documentation clearly describes its purpose.
44-52: LGTM! Well-documented default implementation.
The default implementation provides a sensible default value and the documentation clearly explains the return value semantics, including the special case of -1 for unlimited arguments.
8-11: Verify the existence of DefaultExtraArgument class
The AI summary indicates that the DefaultExtraArgument class has been deleted, but it's still being imported and used in the newInstance method.
✅ Verification successful
The DefaultExtraArgument class exists and is properly referenced
The class DefaultExtraArgument exists at the expected location cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/impl/DefaultExtraArgument.java and is correctly imported and used in the ExtraArgument interface. The AI summary was incorrect about the class being deleted.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if DefaultExtraArgument class still exists
# Search for the DefaultExtraArgument class definition
rg -l "class DefaultExtraArgument"
# Search for references to DefaultExtraArgument
rg "DefaultExtraArgument"
Length of output: 853
16-28: 🛠️ Refactor suggestion
Consider using interface-based implementation
While the factory method pattern is a good practice, the current implementation might need revision:
- If
DefaultExtraArgumentis indeed deleted (as per AI summary), this implementation needs to be updated - Consider using an interface-based implementation to avoid exposing the concrete class
- static ExtraArgument newInstance(@NonNull String name, boolean required) {
- return new DefaultExtraArgument(name, required);
+ static ExtraArgument newInstance(@NonNull String name, boolean required) {
+ return new ExtraArgument() {
+ @Override
+ public String getName() {
+ return name;
+ }
+
+ @Override
+ public boolean isRequired() {
+ return required;
+ }
+ };
}Likely invalid or redundant comment.
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/InvalidArgumentException.java (1)
14-17: LGTM! Well-written class documentation.
The added Javadoc clearly describes the purpose of the exception class and when it's thrown, improving code documentation without affecting functionality.
cli-processor/src/main/java-templates/gov/nist/secauto/metaschema/cli/processor/ProcessorVersion.java (3)
3-5: LGTM!
Package declaration and imports are clean and follow Java conventions.
23-56: LGTM! Clean and correct implementation of IVersionInfo interface.
The implementation is straightforward, properly encapsulated, and follows good practices:
- All methods are properly annotated with @OverRide
- Methods are simple and focused
- No potential null returns
15-21: Verify template variable resolution during build.
The class uses Maven/Git template variables that will be replaced during build time. Ensure these variables are properly configured in the build system.
✅ Verification successful
Template variables are properly configured in the build system
The verification shows that:
- The git-commit-id-maven-plugin is properly configured in the parent pom.xml with version 9.0.1
- The templating-maven-plugin is configured for processing the java-templates
- Both ProcessorVersion.java and MetaschemaJavaVersion.java use consistent template variables
- Maven timestamp property is correctly defined in the build configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Maven properties are defined in pom.xml
# and Git plugin is configured correctly
# Check Maven properties
echo "Checking Maven properties..."
rg -A 5 "project.version|timestamp" pom.xml
# Check Git plugin configuration
echo "Checking Git plugin configuration..."
rg -A 10 "git-commit-id-plugin" pom.xml
Length of output: 2057
Script:
#!/bin/bash
# Let's check for git plugin configuration in all pom.xml files recursively
echo "Checking Git plugin configuration in all pom.xml files..."
fd pom.xml --exec rg -A 10 "git-commit-id-plugin|pl.project13.maven:git-commit-id-plugin" {}
# Also check for any resource filtering or template processing configuration
echo "Checking resource filtering configuration..."
fd pom.xml --exec rg -A 10 "resources.*filtering|maven-resources-plugin" {}
Length of output: 1298
Script:
#!/bin/bash
# Let's check parent pom.xml for plugin management section
echo "Checking parent pom.xml for plugin configurations..."
rg -A 20 "<pluginManagement>" pom.xml
# Let's also check if there are any other java-templates directories that might show similar pattern
echo "Checking for other java-templates directories..."
fd -t d "java-templates"
# Check if there are any other similar version files
echo "Checking for similar version files..."
fd -t f "Version.java" -x rg -l "@git" {}
Length of output: 1839
Script:
#!/bin/bash
# Let's check the content of both version files and the parent pom.xml for git plugin
echo "Checking content of MetaschemaJavaVersion.java..."
cat ./core/src/main/java-templates/gov/nist/secauto/metaschema/core/MetaschemaJavaVersion.java
echo "Checking parent pom.xml for git plugin..."
rg -A 15 "pl.project13.maven|git-commit-id" pom.xml
# Check if there's a build profile that might include the git plugin
echo "Checking build profiles..."
rg -A 10 "<profile>" pom.xml
Length of output: 4086
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/CommandService.java (3)
8-8: LGTM! Good use of utility class for null safety.
The addition of ObjectUtils import enhances null safety in the getCommands method.
24-24: LGTM! Clean singleton initialization.
The use of method reference instead of lambda expression improves readability while maintaining the same functionality.
56-65: Consider documenting potential exceptions and verify command loading.
While the implementation is solid with proper null safety and immutability, consider documenting potential ServiceConfigurationError that could occur during service loading.
/**
* Get the loaded commands.
*
+ * @throws ServiceConfigurationError if there is an error loading the command services
* @return the list of loaded commands
*/Let's verify the command implementations in the codebase:
✅ Verification successful
ServiceConfigurationError documentation is warranted, but service loading appears properly configured
The implementation uses Java's ServiceLoader mechanism correctly, and while we don't see concrete command implementations yet, the architecture is properly set up with abstract base classes (AbstractTerminalCommand, AbstractParentCommand, AbstractCommandExecutor) and the ServiceLoader infrastructure. The ServiceConfigurationError can indeed occur during service loading, making the suggested Javadoc addition valuable.
- The application is properly modularized (has module-info.java files)
- ServiceLoader is correctly used to load ICommand implementations
- Abstract command classes are present as the foundation for concrete implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ICommand implementations and their registration
# Look for ICommand implementations and their registration in service files
echo "Checking for ICommand implementations:"
rg "implements.*ICommand" -l
echo -e "\nChecking for service registration files:"
fd "ICommand$" "src/main/resources/META-INF/services"
Length of output: 711
Script:
#!/bin/bash
# Let's check for concrete command implementations and service registration files with a broader search
echo "Checking for concrete command implementations:"
rg "class.*extends.*(AbstractTerminalCommand|AbstractParentCommand)" -l
echo -e "\nChecking for service registration files in the entire codebase:"
fd "ICommand$" --type f
echo -e "\nChecking for ServiceLoader usage:"
rg "ServiceLoader.*ICommand" -A 2
Length of output: 1604
Script:
#!/bin/bash
# Let's check for module-info.java to verify if this is a modular application
# and also check for any command implementations we might have missed
echo "Checking for module-info.java:"
fd "module-info.java$"
echo -e "\nChecking for command implementations with different pattern:"
rg "class.*Command.*implements" -l
echo -e "\nChecking content of any found module-info.java:"
fd "module-info.java$" -x cat {}
Length of output: 8039
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/MessageExitStatus.java (2)
8-9: LGTM! Appropriate utility imports added
The new imports from the core utility package are well-chosen for handling null checks and collections immutability.
34-35: LGTM! Robust implementation of message arguments handling
The refactored implementation:
- Ensures thread-safety through immutability
- Guarantees non-null values
- Is more concise while maintaining clarity
core/src/test/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapterTest.java (1)
8-21: LGTM! Well-structured imports following best practices.
The imports are properly organized with static assertions and appropriate test utilities. Good use of NonNull annotations for parameter validation.
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractCommandExecutor.java (1)
25-33: LGTM! Good encapsulation and documentation.
The constructor visibility change to protected is appropriate for an abstract base class, preventing direct instantiation while allowing subclassing. The parameter documentation and null-safety annotations are well implemented.
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/object/AbstractAmbiguousTemporal.java (1)
Line range hint 1-52: Overall implementation looks good!
The AbstractAmbiguousTemporal class is well-designed and provides a clean solution for handling temporal values with optional timezones. The implementation:
- Uses proper OO design patterns
- Provides good type safety through generics
- Is well documented
- Handles null safety appropriately
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/ExitCode.java (2)
10-12: LGTM! Clear and concise class-level documentation.
The enum's purpose is well-documented, and the visibility is appropriate for its intended use.
Line range hint 67-89: Verify ExitStatus implementations.
Let's verify that the referenced ExitStatus implementations exist and are properly implemented.
✅ Verification successful
Let me gather more information about the implementations to verify they are properly structured.
Let me check one more thing to verify the AbstractExitStatus implementation since both classes extend it.
ExitStatus implementations are properly structured and used correctly
The verification confirms that:
- Both
NonMessageExitStatusandMessageExitStatusproperly extendAbstractExitStatus - The constructors match the usage in
ExitCode.java - The class hierarchy and implementations are consistent
- The only usages of these classes are within the
ExitCodeclass methods, as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ExitStatus interface and its implementations
echo "Searching for ExitStatus interface and implementations..."
rg --type java -l "interface.*ExitStatus|class.*(NonMessageExitStatus|MessageExitStatus)"
# Verify the constructors match our usage
echo "Verifying constructor signatures..."
ast-grep --pattern 'class $_ {
$_($_ exitCode) {
$$$
}
}'
Length of output: 640
Script:
#!/bin/bash
# Check the content of ExitStatus interface
echo "=== ExitStatus interface ==="
rg -A 5 "interface.*ExitStatus" cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/ExitStatus.java
# Check NonMessageExitStatus implementation
echo -e "\n=== NonMessageExitStatus class ==="
rg -A 10 "class.*NonMessageExitStatus" cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/NonMessageExitStatus.java
# Check MessageExitStatus implementation
echo -e "\n=== MessageExitStatus class ==="
rg -A 10 "class.*MessageExitStatus" cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/MessageExitStatus.java
Length of output: 1339
Script:
#!/bin/bash
# Check AbstractExitStatus implementation
echo "=== AbstractExitStatus class ==="
rg -A 15 "class.*AbstractExitStatus" cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/AbstractExitStatus.java
# Check if there are any other usages of these exit status classes
echo -e "\n=== Usage of ExitStatus classes ==="
rg "new (Non)?MessageExitStatus" --type java
Length of output: 1193
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPAddressItem.java (1)
8-15: LGTM! Clear documentation and proper import.
The interface-level Javadoc and import statement are well-structured and appropriate.
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommandExecutor.java (1)
Line range hint 26-43: Well-implemented factory method!
The static factory method is well-designed with:
- Proper null-safety annotations
- Clear documentation
- Effective use of lambda expressions
- Good separation of concerns
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/ITokenItem.java (3)
9-9: LGTM! Import addition aligns with error handling improvements.
The addition of InvalidTypeMetapathException import supports the enhanced error handling implementation.
15-27: LGTM! Documentation improvements enhance clarity.
The updated Javadoc with the @throws clause provides clear documentation of the error conditions, improving API usability.
29-39: LGTM! Improved error handling with detailed messages.
The enhanced error handling:
- Uses a more specific exception type
- Provides detailed error messages including both the invalid value and the original error
- Properly preserves the cause of the exception
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INcNameItem.java (2)
54-61: Exception handling consistency issue was previously identified
The current implementation still shows inconsistency in exception handling between valueOf and cast methods, as noted in the previous review.
33-38: Clarify the purpose of null parameter in exception constructor
The InvalidTypeMetapathException constructor is called with null as its first parameter. This parameter's purpose should be documented or a meaningful value should be provided.
✅ Verification successful
The null parameter for item is acceptable in this context
Looking at the InvalidTypeMetapathException class implementation, the first parameter is explicitly marked as @Nullable in the constructor, and the class is designed to handle cases where no item is available. The null value is stored in the item field and can be retrieved using the getItem() method which is also marked as @Nullable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of InvalidTypeMetapathException to understand the parameter
ast-grep --pattern 'new InvalidTypeMetapathException($_, $_, $_)'
Length of output: 16510
Script:
#!/bin/bash
# Let's look at the InvalidTypeMetapathException class definition
ast-grep --pattern 'class InvalidTypeMetapathException'
Length of output: 57
Script:
#!/bin/bash
# Let's try to find the InvalidTypeMetapathException class using ripgrep
rg "class InvalidTypeMetapathException" -A 5
Length of output: 736
Script:
#!/bin/bash
# Let's look at the full InvalidTypeMetapathException class implementation
cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/InvalidTypeMetapathException.java
Length of output: 2685
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/MarkupXmlStreamWriter.java (1)
6-6: Verify impact of package relocation
The class has been moved to the impl package. Let's verify that all references have been updated accordingly.
✅ Verification successful
Package relocation is properly handled
The class relocation to the impl package appears to be safe. The only usage found is in AbstractMarkupString which already has the correct import statement referencing the new impl package location. No references to the old package path remain in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old package path
rg "gov\.nist\.secauto\.metaschema\.core\.datatype\.markup\.flexmark\.MarkupXmlStreamWriter" --type java
# Search for direct instantiations of this class to ensure they're only within appropriate contexts
ast-grep --pattern 'new MarkupXmlStreamWriter($$$)'
Length of output: 740
Script:
#!/bin/bash
# Let's check if the class is part of any public API or if it's properly encapsulated
ast-grep --pattern 'class MarkupXmlStreamWriter' --lang java
# Check for any imports of this class
rg "import.*MarkupXmlStreamWriter" --type java
Length of output: 643
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractParentCommand.java (3)
11-11: LGTM: Import addition for null safety
The addition of ObjectUtils import supports the null safety improvements in the code.
54-55: LGTM: Improved null safety
The use of ObjectUtils.notNull ensures consistent null safety behavior.
69-71: Verify unused parameter necessity
The commandLine parameter is marked as unused. If it's truly not needed, consider removing it from the method signature.
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/FixedEmphasisDelimiterProcessor.java (1)
6-6: LGTM! Package relocation improves encapsulation
Moving the class to the .impl package better reflects its implementation nature and follows Java conventions for separating implementation details from the API.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv6AddressItem.java (3)
9-11: LGTM!
The added imports are necessary for the enhanced error handling in the new and updated methods.
21-40: LGTM! Robust error handling implementation.
The new valueOf method is well-implemented with:
- Clear documentation
- Proper use of the MetaschemaDataTypeProvider parser
- Comprehensive error handling that preserves the original error context
- Consistent null-safety annotations
66-73: LGTM! Follows established casting pattern.
The cast implementation aligns with the intentional casting patterns used across atomic type implementations. It includes:
- Efficient type checking with instanceof
- Proper fallback to string conversion
- Comprehensive error handling for both parsing and conversion failures
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPv4AddressItem.java (3)
9-9: LGTM: Import additions support enhanced error handling.
The new imports are appropriately used to support the enhanced error handling in the valueOf and cast methods.
Also applies to: 11-11
16-40: LGTM: Robust valueOf implementation with proper error handling.
The implementation demonstrates several good practices:
- Comprehensive documentation
- Proper null-safety annotations
- Detailed error messages including the original cause
- Appropriate use of the MetaschemaDataTypeProvider for parsing
66-73: LGTM: Cast implementation follows the established pattern.
The cast implementation appropriately handles type conversion with proper error handling:
- Attempts direct casting first for efficiency
- Falls back to string conversion when needed
- Properly wraps exceptions in InvalidValueForCastFunctionException
core/src/test/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateTimeAdapterTest.java (2)
8-25: LGTM! Well-organized imports and clean class setup.
The imports are properly organized and the adapter initialization is clean and straightforward.
71-78: LGTM! Well-structured test method with proper assertions.
The test method is well-implemented with:
- Clear parameter naming
- Proper null-safety annotations
- Grouped assertions for better error reporting
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUriReferenceItem.java (3)
9-11: LGTM: Import statements are appropriate.
The added imports support the enhanced error handling in the valueOf and cast methods.
17-24: LGTM: Well-documented interface definition.
The Javadoc provides clear documentation with appropriate references to standards (RFC2396) and related classes.
73-82: Verify the casting behavior across different item types.
The implementation follows a clear type hierarchy and includes proper error handling. However, it would be beneficial to verify the casting behavior with different item types.
✅ Verification successful
Let me gather more information about the type hierarchy and casting implementations.
Based on the analysis of the codebase, I can now provide a final response about the casting behavior:
The URI reference casting implementation follows the established pattern and is correctly implemented.
The implementation aligns with the type casting patterns used across all atomic items in the codebase:
- It properly handles the type hierarchy (IUriReferenceItem -> IAnyUriItem -> string conversion)
- The error handling matches the standard pattern used by other atomic item implementations
- The casting behavior is consistent with similar atomic types like IAnyUriItem, IIPAddressItem, etc.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all implementations of IAnyAtomicItem to ensure the cast method handles all cases
ast-grep --pattern 'class $_ implements IAnyAtomicItem'
# Search for usages of the cast method to understand the impact
rg -l "IUriReferenceItem.cast"
Length of output: 184
Script:
#!/bin/bash
# Search for implementations of IAnyUriItem to understand the type hierarchy
ast-grep --pattern 'class $_ implements IAnyUriItem'
# Search for other cast methods in atomic items to understand casting patterns
rg "cast\(" -A 5 "core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/"
# Look for test cases related to URI casting
rg -l "UriReference.*[Tt]est"
Length of output: 38442
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IStringItem.java (2)
8-11: LGTM! Imports are appropriate for the implementation.
The added imports support the enhanced type validation and error handling in the valueOf method.
53-58: LGTM! Clean and type-safe implementation.
The cast implementation:
- Uses proper type checking
- Follows the recommended pattern for string conversion
- Has appropriate error handling
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/MetapathConstants.java (4)
15-18: LGTM! Class-level documentation and structure are well-defined.
The class is properly documented and structured as a constants utility class with appropriate PMD suppression.
19-58: LGTM! URI constants are well-structured and properly null-checked.
The URI constants follow a consistent hierarchical pattern and are properly protected against null values using ObjectUtils.requireNonNull.
Line range hint 91-93: LGTM! Constructor properly prevents instantiation.
The private constructor follows the standard utility class pattern.
19-58: Verify consistent usage of namespace URIs.
Let's ensure these namespace URIs are consistently used across function implementations and registrations.
✅ Verification successful
Namespace URIs are used consistently across the codebase
The verification shows that the namespace URIs defined in MetapathConstants.java are used consistently throughout the codebase:
- All function implementations correctly use their designated namespaces:
- Core functions use
NS_METAPATH_FUNCTIONS - Array functions use
NS_METAPATH_FUNCTIONS_ARRAY - Map functions use
NS_METAPATH_FUNCTIONS_MAP - Math functions use
NS_METAPATH_FUNCTIONS_MATH
- Core functions use
- Data type adapters consistently use
NS_METAPATHfor type declarations - No hardcoded URI strings found outside of the constants file
- Proper namespace registration in
StaticContextfor all defined URIs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the namespace URIs in function implementations and registrations
rg -l "http://csrc.nist.gov/ns/metaschema/metapath" --type java
# Search for any hardcoded URIs that should use these constants
rg -l "http://csrc.nist.gov/ns/metaschema/metapath[^\"']*" --type java | \
grep -v "MetapathConstants.java"
Length of output: 255
Script:
#!/bin/bash
# Search for any usage of the namespace constants
rg -A 2 "NS_METAPATH|NS_METAPATH_FUNCTIONS|NS_METAPATH_FUNCTIONS_MATH|NS_METAPATH_FUNCTIONS_ARRAY|NS_METAPATH_FUNCTIONS_MAP" --type java
# Search for function registrations that might use these namespaces
ast-grep --pattern 'register($$$)'
Length of output: 60417
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateTimeAdapter.java (6)
11-11: LGTM: Import change aligns with type refactoring
The addition of AmbiguousDateTime import is necessary for the type system changes.
26-32: LGTM: Enhanced documentation and type system improvements
The changes improve code documentation and implement a more precise type system using AmbiguousDateTime. The Javadoc link to the specification is particularly helpful.
41-41: LGTM: Constructor updated consistently
The constructor change aligns with the new type system.
71-83: LGTM: Well-structured parsing methods
The new private parsing methods are well-designed with:
- Clear separation of concerns
- Proper null-safety checks
- Focused responsibility
88-91: LGTM: Improved string formatting logic
The asString implementation is now more concise while maintaining proper null checks and clear format selection logic.
101-103: LGTM: Consistent type handling
The newItem implementation properly handles the new AmbiguousDateTime type.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IUuidItem.java (2)
9-19: LGTM! Well-structured imports and interface documentation.
The new imports for exception handling are appropriate, and the interface documentation clearly describes its purpose.
Line range hint 21-86: Verify consistency with other atomic item implementations.
The changes align with the PR objectives of improving type safety and error handling. Let's verify the consistency of this pattern across other atomic item implementations.
✅ Verification successful
Let me gather more specific information about the implementation patterns.
Implementation is consistent with other atomic item interfaces
The verification shows that the IUuidItem implementation follows the same pattern consistently used across all atomic item interfaces:
-
The
valueOf(String)method:- Uses the appropriate MetaschemaDataTypeProvider for parsing
- Wraps IllegalArgumentException with InvalidTypeMetapathException
- Provides descriptive error messages
-
The
cast(IAnyAtomicItem)method:- Handles type-checking with instanceof
- Reuses existing instance if already correct type
- Falls back to string conversion
- Properly wraps exceptions in InvalidValueForCastFunctionException
The implementation aligns perfectly with the established patterns seen in other atomic item interfaces like IDecimalItem, IStringItem, IDateItem, etc.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other atomic item implementations follow the same pattern
# Search for similar valueOf and cast implementations in other atomic item interfaces
ast-grep --pattern 'interface I${_}Item {
$$$
static I${_}Item valueOf(@NonNull String value) {
$$$
}
$$$
static I${_}Item cast(@NonNull IAnyAtomicItem item) {
$$$
}
$$$
}'
# Check for consistent exception handling patterns
rg -l "InvalidTypeMetapathException|InvalidValueForCastFunctionException" --type java
Length of output: 4582
Script:
#!/bin/bash
# Check specific implementations of valueOf and cast methods in a few atomic item interfaces
rg -A 10 "static \w+Item valueOf\(@NonNull String value\)" --type java
# Check for consistent exception handling in cast methods
rg -A 10 "static \w+Item cast\(@NonNull IAnyAtomicItem item\)" --type java
Length of output: 64553
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IMarkupItem.java (4)
12-15: LGTM: Import changes are appropriate.
The new imports support the enhanced error handling and implementation references.
31-34: Error handling still needs to be implemented.
The previous review comment about implementing proper error handling in the valueOf(String) method is still applicable.
72-79: LGTM: Well-implemented error handling and type checking.
The refactored implementation:
- Optimizes performance by avoiding unnecessary casting
- Properly handles and wraps exceptions
- Includes helpful comments about exception sources
72-79: Verify consistent error handling across related components.
Let's ensure that similar error handling patterns are used consistently across related components.
✅ Verification successful
Error handling is consistent across atomic item implementations
The error handling in IMarkupItem.cast() follows the standard pattern used consistently across all atomic item implementations:
- Direct instance check with instanceof
- Fallback to string conversion via
valueOf(item.asString()) - Consistent catching of
IllegalStateExceptionandInvalidTypeMetapathException - Uniform wrapping of exceptions in
InvalidValueForCastFunctionException
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar cast methods in related atomic item implementations
ast-grep --pattern 'static $_ cast(@NonNull IAnyAtomicItem $_) {
try {
$$$
} catch ($$$) {
$$$
}
}'
# Search for other valueOf methods that might need similar error handling
rg -p "static \w+ valueOf\(@NonNull String" --type java
Length of output: 3422
Script:
#!/bin/bash
# Let's examine the error handling in a few representative atomic item implementations
rg -A 10 "static \w+ cast\(@NonNull IAnyAtomicItem" --type java
# Also check for InvalidValueForCastFunctionException usage
rg -A 3 "throw new InvalidValueForCastFunctionException" --type java
Length of output: 46803
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBase64BinaryItem.java (4)
9-11: LGTM! Import statements align with error handling improvements.
The new imports support the enhanced error handling mechanisms, which is consistent with the PR's code cleanup objectives.
34-43: Well-implemented error handling with informative messages.
The error handling implementation is robust:
- Safely handles values of any length using Math.min
- Includes both the problematic value preview and original error message
- Properly chains the original exception
71-78: LGTM! Comprehensive error handling in cast method.
The implementation is well-designed:
- Efficiently handles items that are already IBase64BinaryItem
- Provides comprehensive error handling for conversion failures
- Includes helpful comment explaining the IllegalStateException case
37-39: Verify the null parameter in InvalidTypeMetapathException constructor.
The first parameter of the exception constructor is set to null. Please verify if this is intentional or if it should reference the current context/expression.
✅ Verification successful
Based on the search results, I can see a clear pattern in how InvalidTypeMetapathException is used throughout the codebase. The constructor is frequently called with a null first parameter, particularly in cases where the exception is thrown during validation of data types, parsing, or format checking. This is consistent across various atomic type implementations (e.g., Base64Binary, Boolean, DateTime, IPv4Address, etc.) and other utility classes.
The null parameter usage in InvalidTypeMetapathException constructor is intentional and follows the established pattern.
- The first parameter appears to be used for context/item reference, which is set to
nullwhen the error occurs during type validation or parsing, rather than during processing of a specific item. - When the exception is related to a specific item being processed, that item is passed as the first parameter instead of
null(as seen inDefaultFunction.javaandFunctionUtils.java).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other InvalidTypeMetapathException usage patterns
rg -A 2 "new InvalidTypeMetapathException\("
Length of output: 19844
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/FlexmarkFactory.java (2)
15-16: LGTM! Improved modularity with new imports.
The addition of these imports indicates better separation of concerns, moving the emphasis delimiter processing and configuration management into dedicated implementations.
64-64: Verify the singleton configuration instance usage.
The switch to FlexmarkConfiguration.instance() suggests a move towards centralized configuration management. However, we should verify that this singleton instance is properly initialized before use.
✅ Verification successful
Let me gather more information about the FlexmarkConfiguration class implementation to verify its thread-safety and initialization.
Let me check the initialization method and Lazy implementation to ensure thread-safety.
Let me check one final thing about the Lazy implementation since it's crucial for thread-safety.
Based on the gathered information, I can now provide the final response:
The singleton configuration implementation is thread-safe and properly initialized.
The FlexmarkConfiguration implementation uses a thread-safe lazy initialization pattern through:
- A final static field
FLEXMARK_CONFIGinitialized withLazy.lazy() - Proper initialization sequence in
initFlexmarkConfig() - Null-safety checks via
ObjectUtils.notNull()
The usage of FlexmarkConfiguration.instance() is safe and follows best practices for singleton configuration management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the FlexmarkConfiguration initialization and usage pattern
# Look for other direct uses of FlexmarkConfiguration that might need similar updates
# Search for FlexmarkConfiguration usage patterns
echo "Searching for FlexmarkConfiguration usage patterns..."
rg -A 3 "FlexmarkConfiguration\." --type java
# Look for potential race conditions in initialization
echo "Checking for thread-safety patterns..."
ast-grep --pattern 'class FlexmarkConfiguration {
$$$
private static $_ instance;
$$$
}'
Length of output: 1845
Script:
#!/bin/bash
# Search for FlexmarkConfiguration class implementation
echo "Searching for FlexmarkConfiguration class implementation..."
rg -A 20 "class FlexmarkConfiguration" --type java
# Look for instance initialization pattern
echo "Looking for instance initialization..."
ast-grep --pattern 'class FlexmarkConfiguration {
$$$
public static FlexmarkConfiguration instance() {
$$$
}
$$$
}'
Length of output: 3248
Script:
#!/bin/bash
# Search for initFlexmarkConfig implementation
echo "Searching for initFlexmarkConfig implementation..."
rg -A 10 "initFlexmarkConfig" --type java
# Check Lazy class implementation
echo "Checking Lazy class implementation..."
rg -A 5 "class Lazy[<]" --type java
Length of output: 3359
Script:
#!/bin/bash
# Search for Lazy class usage and implementation
echo "Searching for Lazy class..."
rg -l "Lazy" --type java
# Get the full implementation if found
echo "Getting Lazy class implementation..."
rg -B 2 -A 15 "class.*Lazy" --type java
Length of output: 11488
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/CastFunction.java (2)
99-104: Well-designed functional interface with proper documentation
The new ICastExecutor interface is well-structured with:
- Clear and complete documentation
- Proper use of generics for type safety
- Appropriate nullability annotations
- Single responsibility principle adherence
Also applies to: 105-116
Line range hint 1-98: Verify integration with refactored casting operations
The implementation looks solid, but since this is part of a larger refactoring effort moving casting operations, we should verify that all callers have been updated to use the new ICastExecutor pattern.
✅ Verification successful
Let me gather more specific information about the casting implementation to verify the refactoring completeness.
Based on the gathered information, I can now provide the final response:
Refactoring is properly implemented with no remaining legacy patterns
The verification shows:
- The new
ICastExecutorpattern is correctly implemented inCastFunction.java - The old casting pattern in
AbstractDataTypeAdapteris commented out (shown by// public ITEM_TYPE cast(IAnyAtomicItem item)) - No other direct cast implementations were found that need migration
- All related atomic item types are properly aligned with the new casting structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of the new casting approach
# Find potential callers that might need updates
echo "Searching for potential callers of casting operations..."
rg -l "cast.*AtomicItem|AbstractDataTypeAdapter.*cast" --type java
# Look for any remaining old casting patterns that should be migrated
echo "Checking for old casting patterns that should be migrated..."
ast-grep --pattern 'class $_ extends AbstractDataTypeAdapter {
$$$
cast($_, $_) {
$$$
}
$$$
}'
Length of output: 3422
Script:
#!/bin/bash
# Check the AbstractDataTypeAdapter implementation and its usage
echo "Checking AbstractDataTypeAdapter implementation..."
rg -A 10 "class AbstractDataTypeAdapter" core/src/main/java/gov/nist/secauto/metaschema/core/datatype/AbstractDataTypeAdapter.java
echo "\nChecking IDataTypeAdapter interface..."
rg -A 10 "interface IDataTypeAdapter" core/src/main/java/gov/nist/secauto/metaschema/core/datatype/IDataTypeAdapter.java
echo "\nChecking for ICastExecutor usage..."
rg "ICastExecutor" --type java
echo "\nChecking for direct cast method implementations..."
rg "public .* cast\(" --type java
Length of output: 2437
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDateItem.java (4)
38-43: LGTM! Improved error handling.
The changes enhance error reporting by:
- Using a more specific exception type (InvalidTypeMetapathException)
- Including both the invalid value and the original error message
- Maintaining proper null safety
85-102: LGTM! Well-structured type casting implementation.
The cast implementation is robust and well-organized:
- Handles all relevant item types explicitly
- Provides clear error messages
- Properly propagates exceptions with context
- Follows a clear type-checking pattern
55-57: Verify the impact of switching to AmbiguousDate.
The change from Date to AmbiguousDate is significant. Let's ensure all callers have been updated.
✅ Verification successful
The change to AmbiguousDate is properly integrated
Based on the verification results:
- All callers of
IDateItem.valueOf()are using the newAmbiguousDatetype - No references to
java.util.Dateremain in the related files - Both
DateAdapterandDateWithTZAdapterhave been updated to use the new type - The
OperationFunctionsclass uses the new date handling consistently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of java.util.Date with valueOf
rg -l "Date.*valueOf" | xargs rg "import.*java\.util\.Date"
# Search for calls to valueOf to verify they're using AmbiguousDate
ast-grep --pattern 'valueOf($date)'
Length of output: 99
Script:
#!/bin/bash
# Search for calls to valueOf method in IDateItem
rg -A 2 "valueOf\s*\(\s*\w+\s*\)" core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDateItem.java
# Search for references to IDateItem.valueOf
rg -l "IDateItem\.valueOf"
# Search for imports of AmbiguousDate
rg "import.*AmbiguousDate"
# Search for any remaining Date imports in files that might use valueOf
rg -l "valueOf" | xargs rg "import.*java\.util\.Date"
Length of output: 1984
Script:
#!/bin/bash
# Check the implementation of DateAdapter and DateWithTZAdapter for any remaining java.util.Date usage
rg -A 5 "valueOf" core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapter.java
rg -A 5 "valueOf" core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateWithTZAdapter.java
# Check OperationFunctions for any remaining Date usage
rg -A 5 "valueOf" core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/OperationFunctions.java
# Look for any direct usage of java.util.Date in these files
rg "java\.util\.Date" core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapter.java core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateWithTZAdapter.java core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/OperationFunctions.java
Length of output: 10435
106-106: Verify implementing classes provide castAsType.
The removal of the default implementation requires all implementing classes to provide their own castAsType method.
✅ Verification successful
The previous command might have failed due to syntax complexity. Let's try a simpler grep-based approach to:
- Find files containing classes that implement IDateItem
- Show their context to check for castAsType implementation
- Also search for direct castAsType implementations related to IDateItem
Let's look for both the abstract class and its concrete implementations to verify the castAsType implementation.
Let's try to find all relevant files first and then examine their content to locate the implementations.
From the previous output, we found the abstract class and two potential implementing classes. Let's examine their full content to verify the castAsType implementation.
Implementation of castAsType is properly handled
The verification shows that the castAsType method is correctly implemented:
AbstractDateItemprovides the implementation for all its subclasses:
@Override
public IDateItem castAsType(IAnyAtomicItem item) {
return IDateItem.cast(item);
}- Both concrete implementations (
DateWithTimeZoneItemImplandDateWithoutTimeZoneItemImpl) inherit this implementation fromAbstractDateItem, which satisfies the interface requirement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all classes implementing IDateItem
ast-grep --pattern 'class $_ implements $$$IDateItem$$$' | xargs ast-grep --pattern 'castAsType($item)'
Length of output: 105
Script:
#!/bin/bash
# Let's try a different approach to find implementing classes
rg "implements.*IDateItem" -l | while read -r file; do
echo "=== $file ==="
rg "class.*implements.*IDateItem" "$file" -A 20
done
# Also search for direct implementations of castAsType
rg "castAsType.*IDateItem" -A 2
Length of output: 462
Script:
#!/bin/bash
# Let's try to find the AbstractDateItem class content and its implementations
rg -t java "class.*AbstractDateItem.*implements.*IDateItem" -A 50
# Also search for concrete implementations of AbstractDateItem
rg -t java "class.*extends.*AbstractDateItem" -A 20
# Search for castAsType method in these contexts
rg -t java "@Override.*castAsType.*IDateItem" -A 5
Length of output: 174
Script:
#!/bin/bash
# Let's try a more basic search approach
# First find all Java files that might be related to IDateItem
fd -e java | xargs grep -l "IDateItem"
# Then look for the implementation files content
fd -e java | xargs grep -l "DateItem" | while read -r file; do
echo "=== $file ==="
cat "$file" | grep -A 10 "class.*DateItem"
done
Length of output: 7437
Script:
#!/bin/bash
# Let's examine the specific implementation files more closely
echo "=== AbstractDateItem.java ==="
cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/AbstractDateItem.java
echo -e "\n=== DateWithTimeZoneItemImpl.java ==="
cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateWithTimeZoneItemImpl.java
echo -e "\n=== DateWithoutTimeZoneItemImpl.java ==="
cat core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/impl/DateWithoutTimeZoneItemImpl.java
Length of output: 3956
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapter.java (4)
30-36: LGTM! Well-documented class declaration
The class declaration is properly updated to use AmbiguousDate and includes comprehensive Javadoc with proper specification link.
41-51: LGTM! Regex validation retained for proper timezone handling
The complex regex pattern is necessary for proper timezone parsing, as DateTimeFormatter doesn't handle this case correctly. Constructor is properly updated to use AmbiguousDate.
Line range hint 65-87: LGTM! Robust parsing with proper error handling
The parse implementation:
- Maintains proper date validation
- Handles timezones correctly
- Implements thorough error handling with suppressed exceptions
Line range hint 91-106: LGTM! Clean and concise implementation
The methods are well-implemented:
- asString is simplified and more readable
- Proper null checking with ObjectUtils
- Consistent type handling in newItem
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IPositiveIntegerItem.java (6)
10-10: LGTM!
The import of implementation class follows good separation of concerns.
Line range hint 16-26: LGTM!
Clear documentation and proper constant definition with appropriate annotations.
40-47: LGTM!
Improved error handling with more descriptive messages and broader exception catching.
110-119: LGTM!
Well-structured casting logic with proper exception handling. The order of type checks (IPositiveIntegerItem first, then INumericItem) is optimal for performance.
124-124: LGTM!
Good simplification that removes unnecessary intermediate calls while maintaining functionality.
60-60: Verify the impact of broader numeric type acceptance.
The change to accept INumericItem instead of IIntegerItem is intentional as per previous feedback. However, let's verify other usages to ensure consistency.
✅ Verification successful
Based on the search results, I can now generate the final response:
Parameter type change to INumericItem is consistent with codebase patterns
The change to accept INumericItem instead of IIntegerItem in IPositiveIntegerItem.valueOf() aligns with the codebase's type handling patterns. The search results show:
- Similar numeric type handling in
INonNegativeIntegerItemandIIntegerItem - Consistent value conversion patterns across numeric types
- No direct dependencies that would be broken by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other valueOf methods with similar parameter changes
ast-grep --pattern 'valueOf($type value)' | grep -B2 -A2 'INumericItem\|IIntegerItem'
# Search for calls to this method to verify compatibility
rg -t java "valueOf\s*\(\s*\w+\s*\)" --glob "core/src/main/**/*.java"
Length of output: 12119
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IDateTimeItem.java (6)
9-13: LGTM!
The new imports are well-organized and necessary for the enhanced date/time handling functionality.
35-39: LGTM! Improved error handling.
The enhanced error message now includes the original exception's details, making it easier to diagnose parsing issues.
46-54: LGTM! Well-documented timezone handling.
The documentation clearly explains the timezone handling behavior and properly references the AmbiguousDateTime class for additional details.
57-58: LGTM!
The implementation is clean and consistent with the documented behavior.
63-66: LGTM! Clear timezone preservation documentation.
The documentation effectively communicates the timezone preservation behavior and its significance in date/time operations.
89-104: Move item.asString() inside try-catch block.
The previous review comment about moving item.asString() inside the try-catch block is still applicable. This change would ensure proper handling of potential IllegalStateException from the asString() call.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INonNegativeIntegerItem.java (6)
11-12: LGTM: Clear documentation and necessary imports added
The added imports and class-level documentation improve code clarity and maintainability.
Also applies to: 18-20
22-31: LGTM: Enhanced null safety for constants
Good improvement using ObjectUtils.notNull() to ensure constants are never null.
45-53: LGTM: Improved error handling in valueOf(String)
Good improvements:
- Using
MetaschemaDataTypeProviderfor consistent parsing - Enhanced error message with original cause
66-67: Skip review: Previously addressed
The precision loss in conversion is intentional as per previous discussion.
99-99: LGTM: Clearer error message
The updated error message is more descriptive and user-friendly.
116-124: Skip review: Previously addressed
The casting logic design has been previously discussed and confirmed as intentional.
Also applies to: 129-129
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IBooleanItem.java (5)
39-40: LGTM! Improved method signature documentation.
The addition of InvalidTypeMetapathException to the method signature and its documentation improves the API contract clarity.
49-55: LGTM! Enhanced error handling with descriptive messages.
The error handling improvements provide better context by including both the invalid value and the original exception message.
86-104: LGTM! XPath 3.1 compliant implementation.
The implementation follows the XPath 3.1 specification's 3-step casting approach with proper exception handling and suppression.
106-108: LGTM! Clean default implementation.
The default method provides a convenient interface-level casting operation while maintaining consistency with the static cast method.
130-130: LGTM! Consistent use of castAsType.
The update maintains consistency with the new casting approach introduced in the interface.
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/HtmlCodeRenderExtension.java (1)
85-88: Verify source position tracking behavior
The refactoring has changed when srcPos() is called. Previously, it was likely called for all non-customTag cases, but now it's only called in the else branch. This could affect source position tracking when sourcePositionParagraphLines is true.
Consider either:
- Moving srcPos() before the conditional:
if (customTag) {
html.raw(ObjectUtils.notNull(htmlOptions.codeStyleHtmlOpen));
} else if (context.getHtmlOptions().sourcePositionParagraphLines) {
- html.withAttr().tag("code");
+ html.srcPos(node.getText()).withAttr().tag("code");
} else {
html.srcPos(node.getText()).withAttr().tag("code");
}- Or verify that source position tracking is intentionally skipped when
sourcePositionParagraphLinesis true.
Run this script to analyze the usage pattern across the codebase:
core/src/test/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/MarkupParserTest.java (1)
13-14: LGTM! Import changes align with refactoring objectives.
The new imports for XmlMarkupParser and AstCollectingVisitor properly support the refactored markup parsing implementation.
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IAnyUriItem.java (6)
18-20: LGTM: Clear and concise class documentation.
The added class-level Javadoc clearly describes the purpose of the interface.
28-42: LGTM: Improved error handling with domain-specific exception.
The changes enhance error handling by:
- Using domain-specific InvalidTypeMetapathException
- Providing detailed error messages including the invalid value
- Properly preserving the original exception cause
47-53: LGTM: Excellent documentation with practical example.
The added documentation enhances usability by providing:
- Clear usage example with concrete URI
- Precise return type behavior explanation
83-91: LGTM: Robust type-safe casting implementation.
The implementation provides:
- Proper type checking with instanceof
- Comprehensive error handling for both parsing and conversion failures
- Clear exception wrapping maintaining the error chain
93-95: LGTM: Clean default implementation following DRY principle.
The new castAsType method properly delegates to the cast method, avoiding code duplication.
28-42: Verify exception handling in calling code.
The change from IllegalArgumentException to InvalidTypeMetapathException and the addition of InvalidValueForCastFunctionException might require updates in calling code.
Also applies to: 83-91
✅ Verification successful
No impact from exception change - internal error handling
The change from IllegalArgumentException to InvalidTypeMetapathException is safe because:
- The exception is handled internally within the
valueOfmethod itself - All callers use the method in a straightforward way without try-catch blocks
- The callers are internal to the codebase (test utilities and core functions)
- The new exception is more specific and maintains the same error semantics
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential callers that need to be updated
echo "Searching for direct calls to valueOf..."
rg "IAnyUriItem\.valueOf\(" -A 3
echo "Searching for catch blocks that might need updating..."
rg "catch.*IllegalArgumentException.*IAnyUriItem" -A 3
Length of output: 3448
Script:
#!/bin/bash
# Let's check for try-catch blocks around the valueOf calls we found
echo "Checking try-catch blocks in TestUtils.java..."
rg -A 10 "IAnyUriItem\.valueOf" "core/src/test/java/gov/nist/secauto/metaschema/core/metapath/TestUtils.java"
echo "Checking try-catch blocks in FnDocumentAvailableTest.java..."
rg -A 10 "IAnyUriItem\.valueOf" "core/src/test/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnDocumentAvailableTest.java"
echo "Checking try-catch blocks in UriAdapter.java..."
rg -A 10 "IAnyUriItem\.valueOf" "core/src/main/java/gov/nist/secauto/metaschema/core/datatype/adapter/UriAdapter.java"
echo "Checking try-catch blocks in FnBaseUri.java..."
rg -A 10 "IAnyUriItem\.valueOf" "core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnBaseUri.java"
echo "Checking try-catch blocks in FnDocumentUri.java..."
rg -A 10 "IAnyUriItem\.valueOf" "core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnDocumentUri.java"
echo "Checking try-catch blocks in FnStaticBaseUri.java..."
rg -A 10 "IAnyUriItem\.valueOf" "core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnStaticBaseUri.java"
Length of output: 2295
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIntegerItem.java (5)
10-11: LGTM!
The added imports support the new functionality while maintaining proper separation of interface and implementation.
25-25: Good enhancement of null-safety!
The addition of ObjectUtils.notNull() provides compile-time null checking for the constants, which is a good defensive programming practice.
Also applies to: 30-30, 35-35
87-97: LGTM!
The new valueOf(boolean) method is well-implemented with:
- Proper null-safety checks
- Standard boolean-to-number mapping (true→1, false→0)
- Clear documentation
151-153: LGTM!
Clean implementation that properly delegates to the static cast method, reducing duplication.
133-149: Verify the casting behavior across the codebase.
The cast implementation looks good with:
- Clear type conversion hierarchy
- Proper error handling
- Consistent with project's casting patterns
Let's verify the consistency of casting behavior:
✅ Verification successful
The previous pattern might have been too specific. Let's search for cast implementations and casting patterns more broadly to verify the consistency of the casting behavior across the codebase.
Cast implementation is consistent across the codebase
The casting behavior in IIntegerItem follows the same patterns seen across all atomic item implementations:
- Direct type casting when the input is of the target type
- Type-specific conversions for related types (e.g., numeric types)
- Fallback to string conversion with proper error handling
- Consistent exception handling using
InvalidValueForCastFunctionException
All implementations follow similar patterns:
- Simple types (e.g.,
IStringItem,IUuidItem) use direct cast or string conversion - Numeric types (e.g.,
IIntegerItem,IDecimalItem) handle numeric type hierarchies - Date/Time types (e.g.,
IDateTimeItem,IDateItem) handle temporal type conversions - All implementations properly handle error cases with appropriate exceptions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other cast implementations to ensure consistency
ast-grep --pattern 'static $_ cast(@NonNull IAnyAtomicItem item) {
$$$
}'
Length of output: 77
Script:
#!/bin/bash
# Let's try a broader search for cast methods in atomic item classes
rg -l "class.*Item.*implements.*IAtomicItem" | while read -r file; do
echo "=== $file ==="
rg "cast.*IAnyAtomicItem" -A 5 "$file"
done
# Also search for similar casting patterns
rg "static.*cast.*Item" -A 5
Length of output: 19587
metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/GenerateDiagramCommand.java (1)
55-56: LGTM! The refactoring improves consistency.
The transition to ExtraArgument.newInstance factory method is a good improvement that aligns with the broader refactoring effort across command classes.
Let's verify the consistent usage of ExtraArgument.newInstance across other command classes:
✅ Verification successful
The refactoring is consistently applied across all command classes
The verification shows that:
- All command classes in
metaschema-cliconsistently useExtraArgument.newInstance DefaultExtraArgumentonly appears in the implementation class and interface where it's properly encapsulated- The factory method pattern is uniformly applied across:
- GenerateDiagramCommand
- GenerateSchemaCommand
- AbstractValidateContentCommand
- AbstractConvertSubcommand
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of ExtraArgument.newInstance across command classes
# Expected: All command classes should use ExtraArgument.newInstance instead of DefaultExtraArgument
# Search for any remaining usage of DefaultExtraArgument
rg "DefaultExtraArgument"
# Search for consistent usage of ExtraArgument.newInstance in command classes
rg "ExtraArgument\.newInstance" --type java
Length of output: 1929
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/INumericItem.java (3)
8-8: LGTM!
The new imports are appropriate for the added exception handling and utility functionality.
38-45: LGTM! Robust implementation of the cast method.
The implementation:
- Efficiently handles direct casting for INumericItem instances
- Provides fallback to string-based conversion
- Includes proper exception handling and wrapping
132-135: LGTM! Well-structured refactoring of the round method.
The separation into specialized helper methods improves code organization and maintainability.
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommand.java (3)
17-17: LGTM! Import statements are appropriate.
The added imports support the new stream operations and null-safety annotations.
Also applies to: 20-20
22-24: LGTM! Clear and concise interface documentation.
The documentation effectively describes the interface's purpose.
101-116: LGTM! Clear parameter naming and appropriate default implementation.
The parameter rename from cmdLine to commandLine improves readability.
metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/GenerateSchemaCommand.java (1)
51-52: LGTM! Good use of the factory method pattern.
The change from direct instantiation to ExtraArgument.newInstance() improves code maintainability and follows good design practices.
Let's verify that this refactoring pattern is consistently applied across other command classes:
✅ Verification successful
Factory method pattern is consistently applied across command classes
The verification shows that:
- All command classes are using
ExtraArgument.newInstance()consistently - The only direct instantiation of
DefaultExtraArgumentis within the factory method itself (ExtraArgument.java), which is the correct implementation pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of ExtraArgument.newInstance() across command classes
# Expected: All command classes should use the factory method instead of direct instantiation
# Search for any remaining direct instantiations of DefaultExtraArgument
rg "new DefaultExtraArgument" --type java
# Search for the new pattern to confirm consistent usage
rg "ExtraArgument\.newInstance" --type java
Length of output: 1415
core/src/main/java/gov/nist/secauto/metaschema/core/model/xml/ModuleLoader.java (1)
Line range hint 143-143: Consider investigating the TODO comment about system ID handling.
The TODO comment indicates an oddity with system ID formatting that needs investigation. This could have security implications when resolving entities.
Let's check for similar system ID handling patterns in the codebase:
metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/AbstractConvertSubcommand.java (1)
50-51: LGTM! Good use of the factory method pattern.
The switch to ExtraArgument.newInstance() improves encapsulation and maintains consistency with other command classes.
Let's verify the null handling in ExtraArgument.newInstance():
✅ Verification successful
Null handling is properly implemented in ExtraArgument
The code is safe and well-designed:
- The
newInstancefactory method and constructor are annotated with@NonNullfor thenameparameter - The implementation uses proper encapsulation through an interface
- The code is consistently used across multiple command classes
- FindBugs annotations ensure compile-time null checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for null checks in ExtraArgument implementation
ast-grep --pattern 'class ExtraArgument {
$$$
static $_ newInstance($$$) {
$$$
}
$$$
}'
# Look for any related tests
rg -l "ExtraArgument.*test"
Length of output: 131
Script:
#!/bin/bash
# Let's try a different approach to find ExtraArgument class and its implementation
fd "ExtraArgument.java" --type f
# If we find the file, let's examine its content
rg -A 10 "class ExtraArgument"
# Also search for any usage patterns
rg "ExtraArgument\.newInstance"
Length of output: 1481
Script:
#!/bin/bash
# Let's examine the ExtraArgument class implementation
cat "cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ExtraArgument.java"
# Also check the default implementation
cat "cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/impl/DefaultExtraArgument.java"
Length of output: 2359
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnMinMax.java (2)
46-60: Well-structured type definition!
The PRIMITIVE_ITEM_TYPES constant is well-documented and properly encapsulates the supported atomic item types. The use of Set.of() ensures immutability, and @NonNull annotation provides proper null safety.
177-189: Clean and well-structured helper methods!
The convertUntypedItems and countItemTypes methods are well-designed:
- Clear single responsibility
- Proper null safety
- Good encapsulation of functionality
metaschema-cli/src/main/java/gov/nist/secauto/metaschema/cli/commands/AbstractValidateContentCommand.java (2)
60-60: LGTM! Good use of factory method pattern.
The change from direct instantiation to using ExtraArgument.newInstance improves encapsulation and follows better object-oriented design principles. This modification aligns well with similar changes across other command classes, contributing to a more consistent codebase.
60-60: Verify complete migration to factory method.
Since this is an abstract class used by other command implementations, let's ensure the migration is complete.
✅ Verification successful
Let me gather more information to understand the usage context better.
Based on the gathered information, I can now provide the final response:
Migration to factory method is properly implemented
The change is verified to be correct and complete:
ExtraArgument.newInstance()is the factory method defined in the interface- All usages in the codebase consistently use this factory method
- The only occurrence of
new DefaultExtraArgumentis within the factory method itself, which is the correct implementation pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify complete migration from DefaultExtraArgument to ExtraArgument.newInstance
# Check for any remaining DefaultExtraArgument usage
echo "Checking for remaining DefaultExtraArgument usage..."
rg "new DefaultExtraArgument" -l
# Find all classes extending AbstractValidateContentCommand
echo "Finding all implementations of AbstractValidateContentCommand..."
ast-grep --pattern 'class $_ extends AbstractValidateContentCommand'
Length of output: 456
Script:
#!/bin/bash
# Check the content of ExtraArgument.java to understand the DefaultExtraArgument usage
echo "Checking ExtraArgument.java content..."
cat cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ExtraArgument.java
# Search for validate command implementations
echo -e "\nSearching for validate command implementations..."
rg "class.*ValidateCommand" -A 2
# Look for any other ExtraArgument usage patterns
echo -e "\nChecking ExtraArgument usage patterns..."
rg "ExtraArgument\." -A 1
Length of output: 3296
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/MetapathExpression.java (2)
78-80: LGTM! Clear and concise documentation.
The added documentation for CONTEXT_NODE effectively explains its purpose in representing the current context node in Metapath expressions.
Line range hint 50-77: Verify usage of ResultType enum in dependent code.
The addition of the ResultType enum changes how result types are specified in evaluateAs methods. Let's verify that all callers have been updated accordingly.
✅ Verification successful
Based on the search results, I can now generate the final response:
ResultType enum usage is consistent across the codebase.
All calls to evaluateAs methods are properly using the ResultType enum. The search results show:
- All test cases have been updated to use the new
ResultTypeenum - Production code in core modules is using the enum correctly:
ExternalConstraintsModulePostProcessor.javaIIndex.javaAbstractConfigurableMessageConstraint.java
- No legacy or inconsistent usage patterns were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to evaluateAs methods to verify they've been updated to use ResultType
# Search for method calls
echo "Searching for evaluateAs method calls:"
rg -p "evaluateAs\(" --type java
# Search for ResultType enum usage
echo -e "\nVerifying ResultType enum usage:"
rg -p "ResultType\." --type java
Length of output: 23300
core/src/test/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupStringTest.java (1)
21-21: LGTM! Package restructuring aligns with best practices.
Moving AstCollectingVisitor to the impl package improves separation between API and implementation details.
Let's verify this package restructuring pattern across the codebase:
✅ Verification successful
Based on the shell script results, I can now provide a conclusive response about the package restructuring verification:
Package restructuring to impl follows consistent pattern across codebase
The import change for AstCollectingVisitor is part of a broader, well-structured implementation pattern where:
- All flexmark implementation classes are properly placed in the
flexmark.implpackage - No references to the old package location exist
- Similar
implpackage organization is used consistently throughout the codebase for other implementation classes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the package restructuring pattern and ensure consistency
# Check for other classes moved to impl package
rg -g "*.java" "flexmark\.impl\."
# Check for any remaining references to the old package
rg -g "*.java" "flexmark\.AstCollectingVisitor"
# Look for similar package restructuring patterns
fd -e java -x grep -l "impl\." {} \;
Length of output: 13886
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/StaticContext.java (2)
362-362: LGTM! Improved encapsulation of builder field
The visibility change from public to private for useWildcardWhenNamespaceNotDefaulted improves encapsulation while maintaining proper access through the builder pattern.
40-50: LGTM! Namespace prefix changes are consistent
The renaming from XPath to Metapath function prefixes is well-structured and maintains consistency across all function types (standard, math, array, map).
Let's verify the consistency of these namespace changes across the codebase:
✅ Verification successful
Let me verify the namespace declarations in MetapathConstants.java to ensure complete consistency:
Namespace changes are correctly implemented and consistent
The verification confirms:
- All old XPath prefixes have been completely removed
- New Metapath prefixes are consistently defined in MetapathConstants.java
- Namespace URIs are properly structured with a consistent base URI (http://csrc.nist.gov/ns/metaschema/metapath-functions)
- All function-specific namespaces (math, array, map) are correctly derived from the base namespace
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that old XPath prefixes are not used elsewhere and new Metapath prefixes are used consistently
# Check for any remaining references to old XPath prefixes
echo "Checking for old XPath prefixes..."
rg "PREFIX_XPATH_FUNCTIONS"
# Check for consistent usage of new Metapath prefixes
echo "Checking usage of new Metapath prefixes..."
rg "PREFIX_METAPATH_FUNCTIONS"
Length of output: 2147
Script:
#!/bin/bash
# Check the namespace declarations in MetapathConstants.java
echo "Checking namespace declarations..."
rg "NS_METAPATH_FUNCTIONS" core/src/main/java/gov/nist/secauto/metaschema/core/metapath/MetapathConstants.java -B 1 -A 1
# Check if there are any remaining XPath namespace references
echo "Checking for any remaining XPath namespace references..."
rg "NS_XPATH|XPATH_FUNCTIONS" core/src/main/java/gov/nist/secauto/metaschema/core/metapath/
Length of output: 1143
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java (1)
Line range hint 252-307: LGTM! Casting functions properly relocated
The removal of XML Schema namespace casting functions while maintaining Metapath namespace ones aligns well with the PR objective of improving separation between type adapters and Metapath items.
Let's verify that the casting operations have been properly relocated to atomic item implementations:
✅ Verification successful
Let's check the content of FnMinMax.java to understand the casting operation there, and also verify the atomic item implementations more thoroughly.
Casting operations properly relocated with one acceptable exception
The verification confirms that:
- All atomic item types have their casting operations properly implemented in their respective files under the atomic package
- The only instance of casting outside atomic items is in
FnMinMax.java, which is acceptable as it's performing a necessary type conversion of untyped items to decimal items for comparison purposes in the min/max functions
The changes align well with the PR objective of improving separation between type adapters and Metapath items.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that casting operations exist in atomic item implementations
# and that no other files are performing casting operations
# Check for cast methods in atomic item implementations
echo "Checking for cast methods in atomic item implementations:"
ast-grep --pattern 'class $_ implements I${_}Item {
$$$
static $_ cast($_) {
$$$
}
$$$
}'
# Check for potential casting operations outside atomic items
echo "Checking for potential casting operations outside atomic items:"
rg -g '!**/atomic/**' -l 'instanceof.*Item.*\b(cast|convert)'
Length of output: 521
Script:
#!/bin/bash
# Check the casting operation in FnMinMax
echo "Content of FnMinMax.java:"
rg -A 5 -B 5 'instanceof.*Item.*\b(cast|convert)' core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnMinMax.java
# Check atomic item implementations for cast methods
echo -e "\nChecking atomic item implementations for cast methods:"
fd -e java . core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/ --exec rg -l 'static.*cast'
Length of output: 3090
metaschema-testing/src/main/java/gov/nist/secauto/metaschema/model/testing/AbstractTestSuite.java (3)
132-134: LGTM: Parameter addition improves type safety
The addition of the @NonNull bindingContext parameter enhances type safety and makes the dependency explicit.
319-327: LGTM: Proper lazy initialization with null safety
The initialization is well-implemented with proper error handling and null safety checks.
342-342: LGTM: Efficient lazy schema generation
The use of Lazy.lazy for schema generation is an excellent choice as it:
- Prevents unnecessary schema generation
- Maintains thread safety
- Improves performance
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/AbstractAstVisitor.java (1)
82-89: Well-documented class with appropriate annotations!
The added class-level Javadoc clearly describes the purpose of the class and properly documents the generic type parameter. The PMD suppression annotations are justified given the nature of the visitor pattern implementation.
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/HtmlQuoteTagExtension.java (2)
39-42: Well-Documented Class Description
The JavaDoc comments for the HtmlQuoteTagExtension class clearly explain the purpose and functionality of the class, enhancing code readability and maintainability.
Line range hint 161-174: Proper Initialization in DoubleQuoteNode Constructor
The constructor of DoubleQuoteNode correctly initializes the node by copying all relevant properties and child nodes from the provided TypographicQuotes instance, ensuring consistent behavior.
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/FlexmarkConfiguration.java (4)
159-159: Confirm correct merging of configuration options
The method newFlexmarkConfig(@Nullable DataHolder options) now returns options == null ? instance() : DataSet.merge(instance(), options);. Ensure that when options is not null, the merge operation combines the configurations correctly and that there are no unintended side effects.
49-51:
Changing visibility of FLEXMARK_CONFIG to private may affect external access
The static field FLEXMARK_CONFIG has been changed from public to private. If any external code accesses this field directly, this change could lead to compilation errors. Please verify that no external classes rely on direct access to FLEXMARK_CONFIG.
Run the following script to check for external references to FLEXMARK_CONFIG:
#!/bin/bash
# Description: Search for references to `FlexmarkConfiguration.FLEXMARK_CONFIG` outside its own class.
rg 'FlexmarkConfiguration\.FLEXMARK_CONFIG' -t java --glob '!:core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/FlexmarkConfiguration.java'57-60: Ensure thread safety of lazy initialization
The introduction of Lazy<DataSet> for FLEXMARK_CONFIG implements lazy initialization. Please confirm that the Lazy implementation from nl.talsmasoftware.lazy4j is thread-safe to avoid potential concurrency issues when accessing the configuration instance in a multi-threaded environment.
Run the following script to review the usage of Lazy and confirm thread safety:
#!/bin/bash
# Description: Check documentation or source code for `Lazy` regarding thread safety.
# Note: Since we cannot access external documentation or source code in this script,
# please manually verify that `nl.talsmasoftware.lazy4j.Lazy` is thread-safe.
echo "Please verify the thread safety of nl.talsmasoftware.lazy4j.Lazy in its documentation."6-6: Verify package declaration changes and update imports accordingly
Changing the package declaration to gov.nist.secauto.metaschema.core.datatype.markup.flexmark.impl may impact other classes that depend on FlexmarkConfiguration. Please ensure that all references to this class have been updated to reflect the new package.
Run the following script to identify files that may need import updates:
✅ Verification successful
Let me try another search strategy to find any references to FlexmarkConfiguration in the codebase.
No import updates needed - package declaration is consistent
The search results show that all references to FlexmarkConfiguration are already using the correct package path gov.nist.secauto.metaschema.core.datatype.markup.flexmark.impl.FlexmarkConfiguration. The imports in both MarkupLine.java and FlexmarkFactory.java are correctly aligned with the current package declaration, indicating no updates are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all Java files importing FlexmarkConfiguration outside the updated package.
rg 'import\s+gov\.nist\.secauto\.metaschema\.\S+\.FlexmarkConfiguration;' -t java --glob '!:core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/impl/FlexmarkConfiguration.java'
Length of output: 182
Script:
#!/bin/bash
# Search for any references to FlexmarkConfiguration class in Java files
rg "FlexmarkConfiguration" -t java
# Also search for potential fully qualified references
rg "gov\.nist\.secauto\.metaschema.*FlexmarkConfiguration" -t java
Length of output: 1802
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/InsertAnchorExtension.java (3)
283-285: Add missing JavaDoc for InsertAnchorNode class
Great job adding the JavaDoc for the InsertAnchorNode class. This improves code documentation and helps other developers understand the class's purpose.
353-353: Simplify return statement in getSegments() method
Simplifying the return statement enhances code readability and efficiency.
60-62: Verify impact of changing constants' visibility to private
Changing ENABLE_INLINE_INSERT_ANCHORS and ENABLE_RENDERING from public to private may affect external classes that access these constants. Please ensure there are no external dependencies on these fields.
Run the following script to check for external usages:
✅ Verification successful
Since there were no matches found in the search results, this indicates that these constants are not referenced outside of their containing file. Let me verify this with a different search strategy to be thorough.
Let me check the actual usage of these constants in the files that import or reference the class.
Based on the comprehensive search results, I can now provide a final response:
Changing visibility to private is safe
The constants ENABLE_INLINE_INSERT_ANCHORS and ENABLE_RENDERING are not referenced anywhere outside their containing class. While several files import and use InsertAnchorExtension, none of them access these specific DataKey constants directly. The change from public to private visibility is safe to proceed with.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for external usages of ENABLE_INLINE_INSERT_ANCHORS and ENABLE_RENDERING.
# Exclude the current file
current_file='core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/InsertAnchorExtension.java'
# Search for ENABLE_INLINE_INSERT_ANCHORS
rg --fixed-strings 'ENABLE_INLINE_INSERT_ANCHORS' --glob "!$current_file"
# Search for ENABLE_RENDERING
rg --fixed-strings 'ENABLE_RENDERING' --glob "!$current_file"
Length of output: 434
Script:
#!/bin/bash
# Try a broader search for any file that might use these constants
# Look for any import statements referencing InsertAnchorExtension
rg "import.*InsertAnchorExtension" -l
# Also check for any direct references to the class name
rg "InsertAnchorExtension\." -l --glob "*.java" --glob "!core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/flexmark/InsertAnchorExtension.java"
Length of output: 1583
Script:
#!/bin/bash
# Check the actual usage of these constants in the identified files
for file in $(rg "InsertAnchorExtension\." -l --glob "*.java"); do
echo "=== $file ==="
rg -A 2 -B 2 "ENABLE_INLINE_INSERT_ANCHORS|ENABLE_RENDERING" "$file"
done
# Also check how these DataKey constants are typically used in the codebase
rg "DataKey.*get\(" -A 2 -B 2
Length of output: 3223
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessor.java (1)
462-465: Ensure proper exception handling when validating extra arguments
In the processCommand method, when calling targetCommand.validateExtraArguments, consider catching specific exceptions that could be thrown and handle them appropriately. This ensures that any issues with argument validation are reported clearly to the user.
Review the exception handling logic to ensure all potential exceptions are accounted for.
...c/main/java/gov/nist/secauto/metaschema/cli/processor/command/impl/DefaultExtraArgument.java
Show resolved
Hide resolved
...c/main/java/gov/nist/secauto/metaschema/cli/processor/command/impl/DefaultExtraArgument.java
Show resolved
Hide resolved
core/src/test/java/gov/nist/secauto/metaschema/core/datatype/adapter/DateAdapterTest.java
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/item/atomic/IIPAddressItem.java
Outdated
Show resolved
Hide resolved
...src/main/java/gov/nist/secauto/metaschema/cli/processor/command/AbstractTerminalCommand.java
Show resolved
Hide resolved
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/command/ICommand.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/model/xml/ModuleLoader.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/antlr/AbstractAstVisitor.java
Outdated
Show resolved
Hide resolved
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessor.java
Outdated
Show resolved
Hide resolved
cli-processor/src/main/java/gov/nist/secauto/metaschema/cli/processor/CLIProcessor.java
Show resolved
Hide resolved
…be more explict. Improved value creation methods in date and date/time item classes. Added many Javadocs.
…ed for a case statement. This should result in cleaner code and better performance overall.
|
@aj-stein-gsa I made a few more changes. Can you re-review? |
|
@coderabbitai Please suggest new class-level Javadocs for undocumented classes in the gov.nist.secauto.metaschema.core package hierarchy. |
|
Oops, something went wrong! Please try again later. 🐰 💔 |
david-waltermire
merged commit be910a9
into
metaschema-framework:develop
Committer Notes
Adding Javadocs; addressing compile, PMD, and spotbugs warnings; and refactoring code for better organization and easier maintenance.
These changes include:
All Submissions:
By submitting a pull request, you are agreeing to provide this contribution under the CC0 1.0 Universal public domain dedication.
Changes to Core Features:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
AmbiguousDateandAmbiguousDateTime.Bug Fixes
Documentation
Refactor