Add fn:substring to Metapath function library
#142
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📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several updates across multiple files, primarily focusing on version updates for various GitHub Actions in workflow files, modifications in the POM files for multiple projects, and enhancements to the functionality of specific classes and interfaces. New inputs for configurable link checking are added, along with the introduction of a new substring function in the Metapath library. Additionally, a new submodule is declared, and documentation is updated to reflect changes in the SARIF module. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (17)
core/src/test/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnSubstringTest.java (2)
25-30: Consider adding more test cases for comprehensive testing.The
provideValuesmethod is well-structured, but it currently only includes one test case. To ensure thorough testing of thesubstringfunction, consider adding more test cases that cover:
- Substrings from the beginning of the string
- Substrings from the end of the string
- Edge cases (e.g., empty string, single character)
- Invalid inputs (e.g., start index out of bounds, negative length)
This will help validate the function's behavior across various scenarios.
32-39: Consider a more specific test method name.The test method is well-structured and correctly uses parameterized tests. However, the name
testExpressionis quite generic. Consider renaming it to something more specific liketestSubstringFunctionto clearly indicate what's being tested.Other than that, the method looks good:
- Correct use of
@ParameterizedTestand@MethodSource.- Proper compilation and evaluation of the Metapath expression.
- Appropriate use of
@NonNullannotations..lycheeignore (2)
Line range hint
38-41: Address remaining items in the ignore list.Thank you for cleaning up the
.lycheeignorefile. There are a couple of items that still need attention:
The comment "prone to long timeouts" for certain URLs suggests that these might be causing issues with the link checker. Consider investigating if there's a way to handle these URLs without ignoring them completely, such as increasing the timeout limit for the link checker if possible.
The TODO comment "fix later" for
https://github.com/metaschema-framework/liboscal-java/indicates that this URL needs attention. Please create an issue to track this TODO item and ensure it's addressed in the future.To improve the maintainability of the
.lycheeignorefile, consider the following suggestions:
- Group URLs by reason for ignoring (e.g., timeout issues, known valid but slow, temporary ignores).
- Add comments explaining the rationale for each ignored URL or group of URLs.
- Implement a regular review process for this file to ensure it stays up-to-date and relevant.
These steps will help maintain the effectiveness of your link checking process and make it easier for contributors to understand and manage the ignored URLs.
Issues Found with Removed URLs in
.lycheeignoreThe following removed URLs are still referenced in the project:
- THIRD_PARTY_LICENSES.md
https://code.google.com/p/atinject/https://codehaus-plexus.github.io/plexus-cipher/https://codehaus-plexus.github.io/plexus-pom/plexus-build-api/https://codehaus-plexus.github.io/plexus-utils/https://codehaus-plexus.github.io/plexus-sec-dispatcher/https://docs.pmd-code.org/https://errorprone.info/error_prone_annotationshttp://forge.sonatype.com/spice-parent/plexus-build-api/https://github.com/google/guava/failureaccesshttps://github.com/google/guava/guavahttps://github.com/google/guava/listenablefuturehttps://github.com/google/guice/guicehttps://github.com/hamcrest/JavaHamcrest/hamcrest-corehttps://github.com/vsch/flexmark-java/flexmarkhttps://github.com/vsch/flexmark-java/flexmark-ext-emojihttps://github.com/vsch/flexmark-java/flexmark-ext-escaped-characterhttps://github.com/vsch/flexmark-java/flexmark-ext-gfm-strikethroughhttps://github.com/vsch/flexmark-java/flexmark-ext-inshttps://github.com/vsch/flexmark-java/flexmark-ext-superscripthttps://github.com/vsch/flexmark-java/flexmark-ext-tableshttps://github.com/vsch/flexmark-java/flexmark-ext-typographichttps://github.com/vsch/flexmark-java/flexmark-ext-wikilinkhttps://github.com/vsch/flexmark-java/flexmark-html2md-converterhttps://github.com/vsch/flexmark-java/flexmark-jira-converterhttps://github.com/vsch/flexmark-java/flexmark-utilhttps://github.com/vsch/flexmark-java/flexmark-util-asthttps://github.com/vsch/flexmark-java/flexmark-util-builderhttps://github.com/vsch/flexmark-java/flexmark-util-collectionhttps://github.com/vsch/flexmark-java/flexmark-util-datahttps://github.com/vsch/flexmark-java/flexmark-util-dependencyhttps://github.com/vsch/flexmark-java/flexmark-util-formathttps://github.com/vsch/flexmark-java/flexmark-util-htmlhttps://github.com/vsch/flexmark-java/flexmark-util-mischttps://github.com/vsch/flexmark-java/flexmark-util-optionshttps://github.com/vsch/flexmark-java/flexmark-util-sequencehttps://github.com/vsch/flexmark-java/flexmark-util-visitor- core/src/test/java/gov/nist/secauto/metaschema/core/datatype/markup/MarkupStringTest.java
https://github.com/vsch/flexmark-java/issues/422Removing these URLs from
.lycheeignorewill cause the link checker to flag them as broken. Please review these references to determine whether to update, retain in the ignore list, or remove them from the codebase.🔗 Analysis chain
Line range hint
1-17: Approve removal of outdated URLs from ignore list.The removal of these URLs from the
.lycheeignorefile is a positive change. It suggests that either these links are no longer used in the project or that they are now expected to be valid and should be checked by the link checker.This change will improve the accuracy of the link checking process and help maintain the overall quality of the documentation and references in the project.
To ensure this change doesn't introduce any issues, please run the following script to verify if any of these removed URLs are still present in the project:
This script will search for the removed URLs in various file types (config files, markdown, Java source, and text files) throughout the project. If any matches are found, please review them to ensure that removing these URLs from the ignore list won't cause any issues.
Also applies to: 19-37
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if any of the removed URLs are still present in the project # List of removed URLs (simplified for brevity) removed_urls=( "https://bytebuddy.net/byte-buddy" "https://chronicle.software/java-parent-pom/compiler" "http://code.google.com/p/" "https://code.revelc.net/revelc/formatter-maven-plugin" "https://codehaus-plexus.github.io/plexus-cipher/" "https://codehaus-plexus.github.io/plexus-pom/plexus-build-api/" "https://codehaus-plexus.github.io/plexus-pom/plexus-utils/" "https://codehaus-plexus.github.io/plexus-sec-dispatcher/" "https://docs.pmd-code.org/" "https://errorprone.info/error_prone_annotations" "http://forge.sonatype.com/spice-parent/plexus-build-api/" "https://github.com/google/guava/" "https://github.com/google/guice/guice" "https://github.com/hamcrest/JavaHamcrest/hamcrest-core" "https://github.com/javaparser/" "https://github.com/vsch/flexmark-java/" "https://glassfish.dev.java.net/" "http://nexus.sonatype.org/oss-repository-hosting.html/" "https://projects.eclipse.org/projects/eclipse.jdt" "https://github.com/metaschema-framework/liboscal-java/" "https://opensource.org/licenses/Apache-2.0,https://opensource.org/licenses/EPL-2.0" ) echo "Checking for removed URLs in the project..." for url in "${removed_urls[@]}"; do echo "Searching for: $url" rg --type-add 'config:*.{xml,properties,yml,yaml}' -t config -t md -t java -t txt "$url" done echo "Search complete. Please review any matches found above."Length of output: 3483
Script:
#!/bin/bash # Description: Check if any of the removed URLs are still present in the project # List of removed URLs removed_urls=( "https://bytebuddy.net/byte-buddy" "https://chronicle.software/java-parent-pom/compiler" "http://code.google.com/p/" "https://code.revelc.net/revelc/formatter-maven-plugin" "https://codehaus-plexus.github.io/plexus-cipher/" "https://codehaus-plexus.github.io/plexus-pom/plexus-build-api/" "https://codehaus-plexus.github.io/plexus-pom/plexus-utils/" "https://codehaus-plexus.github.io/plexus-sec-dispatcher/" "https://docs.pmd-code.org/" "https://errorprone.info/error_prone_annotations" "http://forge.sonatype.com/spice-parent/plexus-build-api/" "https://github.com/google/guava/" "https://github.com/google/guice/guice" "https://github.com/hamcrest/JavaHamcrest/hamcrest-core" "https://github.com/javaparser/" "https://github.com/vsch/flexmark-java/" "https://glassfish.dev.java.net/" "http://nexus.sonatype.org/oss-repository-hosting.html/" "https://projects.eclipse.org/projects/eclipse.jdt" "https://github.com/metaschema-framework/liboscal-java/" "https://opensource.org/licenses/Apache-2.0" "https://opensource.org/licenses/EPL-2.0" ) echo "Checking for removed URLs in the project..." for url in "${removed_urls[@]}"; do echo "Searching for: $url" rg --type-add 'config:*.{xml,properties,yml,yaml}' -t config -t md -t java -t txt "$url" done echo "Search complete. Please review any matches found above."Length of output: 15027
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnSubstring.java (1)
66-78: Consider removing @SuppressWarnings("unused") annotationThe execute method seems to be correctly implemented, handling the function execution and performing proper null checks. However, the @SuppressWarnings("unused") annotation might not be necessary if this method is actually used by the function handler.
If this method is indeed used (which it appears to be, given it's passed to the function handler in the SIGNATURE_THREE_ARG), consider removing the @SuppressWarnings("unused") annotation to improve code clarity.
databind-metaschema/src/test/java/gov/nist/secauto/metaschema/modules/sarif/SarifValidationHandlerTest.java (2)
8-36: Consider optimizing imports.The import statements look appropriate for the test class. However, consider using wildcard imports for packages with multiple imports to improve readability. For example:
import gov.nist.secauto.metaschema.core.model.*; import gov.nist.secauto.metaschema.core.model.constraint.*; import dev.harrel.jsonschema.*;This change can make the import section more concise without sacrificing clarity.
38-40: Consider making the test class public.While the class is correctly set up with JMock, it's not declared as public. Some test runners might have issues with non-public test classes. Consider changing the class declaration to:
public class SarifValidationHandlerTest {This ensures maximum compatibility with different test runners and IDEs.
.github/workflows/build.yml (4)
Line range hint
13-24: LGTM! Consider adding input validation.The new inputs
linkcheck_fail_on_errorandlinkcheck_create_issueare well-defined and provide useful configuration options for the link checking process.Consider adding input validation to ensure that the provided values are always boolean. You can use the
github-scriptaction to validate the inputs at the beginning of the workflow:- name: Validate inputs if: github.event_name == 'workflow_dispatch' uses: actions/github-script@v6 with: script: | const inputs = ['linkcheck_fail_on_error', 'linkcheck_create_issue']; inputs.forEach(input => { if (typeof github.event.inputs[input] !== 'boolean') { core.setFailed(`Input ${input} must be a boolean value.`); } });
43-43: LGTM! Consider updating remaining actions.The updates to action versions are good for security and feature improvements.
Consider updating the versions of the remaining actions that haven't been changed in this PR, such as:
actions/upload-artifactstCarolas/setup-mavenlycheeverse/lychee-actionactions/github-scriptThis will ensure all actions are using the latest stable versions.
Also applies to: 55-55, 62-62, 73-73, 83-83, 95-95, 136-136
61-62: LGTM! Consider expanding CodeQL analysis coverage.The changes to the CodeQL analysis configuration are good. Limiting the analysis to push events can help reduce unnecessary runs, and always uploading results for the 'develop' branch ensures a consistent security baseline.
Consider expanding the CodeQL analysis to run on pull requests targeting the 'develop' branch as well. This can help catch potential security issues before they are merged:
- name: Initialize CodeQL if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.base_ref == 'develop') uses: github/codeql-action/init@461ef6c76dfe95d5c364de2f431ddbd31a417628 with: languages: java # ... (other steps) - name: Perform CodeQL Analysis if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.base_ref == 'develop') uses: github/codeql-action/analyze@461ef6c76dfe95d5c364de2f431ddbd31a417628 with: upload: ${{ github.ref_name == 'develop' || (github.event_name == 'pull_request' && github.base_ref == 'develop') }}Also applies to: 72-75
Line range hint
25-28: LGTM! Consider improving error handling.The changes to the link checker configuration are good. Using environment variables for configuration provides more flexibility and allows the use of workflow dispatch inputs.
Consider adding a step to echo the link checker exit code for better visibility:
- name: Echo link checker exit code if: always() run: echo "Link checker exit code: ${{ env.lychee_exit_code }}"Also, consider using
failure()instead of!cancelled()in the conditional statements for creating issues and failing the workflow. This will ensure these steps run only if the link checker actually failed:- name: Create issue if bad links detected if: ${{ failure() && env.lychee_exit_code != 0 && env.INPUT_ISSUE_ON_ERROR == 'true' }} uses: peter-evans/create-issue-from-file@e8ef132d6df98ed982188e460ebb3b5d4ef3a9cd with: title: Scheduled Check of Website Content Found Bad Hyperlinks content-filepath: ./lychee/out.md labels: | bug documentation - name: Fail on link check error if: ${{ failure() && env.lychee_exit_code != 0 && env.INPUT_FAIL_ON_ERROR == 'true' }} uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea with: script: | core.setFailed('Link checker detected broken or invalid links, read attached report.')Also applies to: 127-145
core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/IMarkupString.java (1)
116-122: Consider updating documentation and usage examples.The addition of the
toText()method is a valuable enhancement to theIMarkupStringinterface. It provides users with a new way to represent markup content as plain text, complementing the existing HTML, XHTML, and Markdown conversions.To ensure proper adoption and usage of this new feature:
- Update any relevant documentation to include information about the
toText()method.- Consider adding usage examples in the project's documentation or README to demonstrate how to use this new method alongside existing conversion methods.
- Ensure that all implementing classes of
IMarkupStringare updated to include this new method.core/src/main/java/gov/nist/secauto/metaschema/core/model/constraint/ConstraintValidationFinding.java (1)
296-296: Improved assertion message, consider standardizing across the class.The addition of the error message "Kind must not be null" to the assertion is a good improvement. It enhances debuggability by providing a clear description of the failure condition.
Consider applying similar descriptive messages to other assertions in this class for consistency. For example, you could update the assertion for
subjectson line 294:-assert subjects != null; +assert subjects != null : "Subjects must not be null";This would maintain a consistent style across all assertions in the
build()method.databind-metaschema/pom.xml (2)
49-53: New test dependency added correctly, but version should be specified.The addition of
jmock-junit5as a test dependency is good for enhancing the project's testing capabilities. However, it's recommended to specify the version of the dependency to ensure consistency and reproducibility of builds.Consider adding a version specification for the
jmock-junit5dependency, like this:<dependency> <groupId>org.jmock</groupId> <artifactId>jmock-junit5</artifactId> <version>2.12.0</version> <scope>test</scope> </dependency>Replace
2.12.0with the desired version.
54-58: New JSON Schema dependency added correctly.The addition of the
json-schemadependency is good for adding JSON Schema validation capabilities to the project. The version is correctly specified.Consider specifying the scope of this dependency if it's not needed for all build phases. For example, if it's only used for testing, you could add:
<scope>test</scope>If it's needed for runtime, you can leave it as is or explicitly add:
<scope>compile</scope>core/src/main/java/gov/nist/secauto/metaschema/core/datatype/markup/AbstractMarkupString.java (1)
211-214: LGTM: toText() method implementation with a minor suggestionThe
toText()method is well-implemented, usingTextCollectingVisitorto extract plain text from the markup structure. It aligns with the expected changes and follows good practices.Consider adding a Javadoc comment to describe the method's purpose and return value. For example:
/** * Extracts and returns the plain text content from the document. * * @return A String containing the plain text content of the document. */ @Override public String toText() { return new TextCollectingVisitor().collectAndGetText(document); }core/pom.xml (1)
Line range hint
1-385: Overall assessment: Changes are consistent and appropriate.The updates to the parent version and SCM tag in this POM file are consistent with each other and align with the ongoing development indicated in the PR objectives. These changes correctly set up the project for the introduction of new functionality (specifically, the
fn:substringfunction mentioned in the PR summary).As a minor suggestion, consider updating the project documentation (such as README files) to reflect the development status and any new features being added. This wasn't done according to the PR checklist, but it would be beneficial for other developers and users of the project.
🛑 Comments failed to post (7)
.github/workflows/release.yml (1)
89-89: 💡 Codebase verification
❌ Issue Found: CNAME File Not Created
The
CNAMEfile was not found in the expected location (target/staging/CNAME) or anywhere else in the repository. This indicates that the workflow step responsible for creating the CNAME file may not be executing correctly.Action Items:
- Review the Workflow Configuration: Ensure that the step to create the
CNAMEfile is correctly defined in.github/workflows/release.yml.- Check GitHub Actions Logs: Investigate the workflow run logs for any errors or warnings related to the CNAME file creation.
- Verify Directory Structure: Confirm that the
target/staging/directory exists and has the appropriate permissions for file creation.- Enhance Error Handling: Modify the workflow to include checks or error messages if the CNAME file creation fails.
🔗 Analysis chain
LGTM: Addition of CNAME file for custom domain
The addition of a CNAME file with the domain "metaschema-java.metaschema.dev" is appropriate for setting up a custom domain for the project website. This is a good practice for improving the visibility and professionalism of the project.
To ensure the custom domain is properly configured, please run the following verification script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the custom domain configuration # Test: Check if the CNAME file is created in the correct location echo "Checking for CNAME file:" cat target/staging/CNAME # Test: Verify the domain name in the CNAME file echo "Verifying domain name:" grep -q "metaschema-java.metaschema.dev" target/staging/CNAME && echo "Domain name is correct" || echo "Domain name is incorrect" # Note: Further DNS configuration might be required on the domain registrar's side echo "Remember to configure DNS settings for the custom domain at your domain registrar."Length of output: 543
Script:
#!/bin/bash # Description: Search for all instances of CNAME files in the repository echo "Searching for CNAME files in the repository:" fd "CNAME"Length of output: 109
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnSubstring.java (1)
92-97:
⚠️ Potential issueEnhance substring method for robustness and XPath compliance
The substring method implementation needs improvements to handle edge cases and fully comply with XPath specifications:
- Add bounds checking to prevent StringIndexOutOfBoundsException.
- Handle negative or zero values for start and length as per XPath specifications.
- Implement proper rounding for non-integer values if they are allowed.
Consider refactoring the method as follows:
@NonNull public static IStringItem substring(@NonNull IStringItem sourceString, @NonNull IIntegerItem start, @NonNull IIntegerItem length) { String source = sourceString.toString(); int sourceLength = source.length(); // XPath uses 1-based indexing, so subtract 1 for 0-based Java indexing int startIndex = Math.max(start.asInteger().intValue() - 1, 0); // Handle negative or zero length int lengthValue = length.asInteger().intValue(); int endIndex = lengthValue <= 0 ? startIndex : Math.min(startIndex + lengthValue, sourceLength); // Ensure startIndex is not greater than endIndex startIndex = Math.min(startIndex, endIndex); return IStringItem.valueOf(source.substring(startIndex, endIndex)); }This implementation ensures that:
- The method never throws StringIndexOutOfBoundsException.
- It handles negative or zero values for start and length.
- It correctly handles cases where the requested substring extends beyond the end of the string.
Please verify this against the exact XPath 3.1 specifications to ensure full compliance.
databind-metaschema/src/test/java/gov/nist/secauto/metaschema/modules/sarif/SarifValidationHandlerTest.java (2)
42-92: 🛠️ Refactor suggestion
Enhance mock setup for better readability and maintainability.
The mock setup is comprehensive, but it can be improved:
Consider extracting mock setup into separate methods for each mock object. This will improve readability and make the test easier to maintain.
Use constants for repeated values like "0.0.0" or 42 to make the test more maintainable.
Consider using a builder pattern for complex objects like
ConstraintValidationFindingto make the setup more readable.Example refactoring:
private void setupVersionInfoMock(IVersionInfo versionInfo) { context.checking(new Expectations() {{ allowing(versionInfo).getName(); will(returnValue("test")); allowing(versionInfo).getVersion(); will(returnValue(VERSION)); }}); } private static final String VERSION = "0.0.0"; private static final int LINE_NUMBER = 42;These changes will make the test setup more modular and easier to understand and maintain.
93-121: 🛠️ Refactor suggestion
Improve SARIF validation logic and error handling.
The SARIF validation logic is good, but consider the following improvements:
Clean up the generated file after the test:
try { // ... existing code ... } finally { Files.deleteIfExists(sarifFile); }Use a
StringJoinerfor more efficient error message construction:StringJoiner sj = new StringJoiner("\n"); for (dev.harrel.jsonschema.Error finding : result.getErrors()) { sj.add(String.format("[%s]%s %s for schema '%s'", finding.getInstanceLocation(), finding.getKeyword() == null ? "" : " " + finding.getKeyword() + ":", finding.getError(), finding.getSchemaLocation())); }Improve the assertion message:
assertTrue(result.isValid(), () -> "SARIF output failed schema validation. Errors:\n" + sj.toString());These changes will make the test more robust, efficient, and informative.
databind-metaschema/pom.xml (1)
170-170: 💡 Codebase verification
Old
metaschemaDirPaths Still PresentSeveral outdated
metaschemaDirpaths are still present indatabind-metaschema/pom.xml:
- Line 136:
${project.basedir}/../core/metaschema/schema/metaschema- Line 153:
${project.basedir}/../core/metaschema/schema/metaschemaPlease remove or update these old paths to ensure consistency and prevent potential conflicts.
🔗 Analysis chain
Metaschema directory updated for SARIF codegen.
The
metaschemaDirconfiguration has been updated to point to a new location for SARIF schemas. This change suggests a project restructure to improve modularity.Please verify that the new path
${project.basedir}/modules/sarifis correct and contains the necessary SARIF schema files. You can run the following script to check:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of SARIF schema files in the new location # Check if the directory exists if [ -d "modules/sarif" ]; then echo "Directory modules/sarif exists." # List XML files in the directory echo "XML files in modules/sarif:" find modules/sarif -name "*.xml" -type f else echo "Error: Directory modules/sarif does not exist." fiLength of output: 1040
CONTRIBUTING.md (1)
78-78:
⚠️ Potential issueFix typo in subsection title
There's a small typo in the subsection title where "functions" is repeated.
Please apply the following change:
-### Core metaschema functions functions +### Core metaschema functions📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.### Core metaschema functions🧰 Tools
🪛 LanguageTool
[duplication] ~78-~78: Possible typo: you repeated a word
Context: ...eloper information ### Core metaschema functions functions The Metaschema [specification](https:/...(ENGLISH_WORD_REPEAT_RULE)
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java (1)
149-153:
⚠️ Potential issueUpdate the comment to reflect the correct function
The addition of
FnSubstring.SIGNATURE_THREE_ARGis correct and aligns with the PR objective of introducing thefn:substringfunction. However, the comment above this line is incorrect.Please update the comment as follows:
- // P0: https://www.w3.org/TR/xpath-functions-31/#func-string + // https://www.w3.org/TR/xpath-functions-31/#func-substring registerFunction(FnSubstring.SIGNATURE_THREE_ARG);This change will correctly link to the XPath specification for the
substringfunction, which is what's being implemented here.The registration of
FnSubstring.SIGNATURE_THREE_ARGis approved. This addition enhances the function library by supporting a three-argument version of the substring function, which is consistent with the XPath specification.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// https://www.w3.org/TR/xpath-functions-31/#func-substring registerFunction(FnSubstring.SIGNATURE_THREE_ARG); // P1: https://www.w3.org/TR/xpath-functions-31/#func-string-join // P1: https://www.w3.org/TR/xpath-functions-31/#func-string-length // P1: https://www.w3.org/TR/xpath-functions-31/#func-subsequence // https://www.w3.org/TR/xpath-functions-31/#func-substring
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnSubstring.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnSubstring.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnSubstring.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnSubstring.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnSubstring.java
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnSubstring.java
Outdated
Show resolved
Hide resolved
...rc/test/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnSubstringTest.java
Outdated
Show resolved
Hide resolved
|
Hey @david-waltermire! I know I have a lot to do and the integer data types and lazy implementation was because I needed time to think I'll handle your very welcome feedback soon enough! |
.../java/gov/nist/secauto/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java
Show resolved
Hide resolved
aj-stein-gsa
force-pushed
the
132-fn-substring
branch
from
1d75d5b
to
53bc8f7
Compare
.../java/gov/nist/secauto/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java
Outdated
Show resolved
Hide resolved
.../java/gov/nist/secauto/metaschema/core/metapath/function/library/DefaultFunctionLibrary.java
Outdated
Show resolved
Hide resolved
core/src/main/java/gov/nist/secauto/metaschema/core/metapath/function/library/FnSubstring.java
Outdated
Show resolved
Hide resolved
aj-stein-gsa
force-pushed
the
132-fn-substring
branch
2 times, most recently
from
0d868b6
to
c69bc95
Compare
Co-authored-by: David Waltermire <[email protected]>
Co-authored-by: David Waltermire <[email protected]>
… args Co-authored-by: David Waltermire <[email protected]>
We fixed some of that in early PR review, the function signatures will obviously need those corrected too.
…riented unit tests until double is implemented.
aj-stein-gsa
force-pushed
the
132-fn-substring
branch
from
c69bc95
to
ccc86f4
Compare
|
So either there is some configuration issue with Javadocs or maybe I am now in charge of writing a lot of docs for those functions, per the build errors? |
…fn-substring-2 Fixed Javadoc generation issue
So, @david-waltermire, obviously the function signature slip ups were on me, but how did only updating the Javadoc plugin for Maven fix those issues? Or does not compiling the function correctly in one place trickle down code gen problems so all related docs fail to build? |
Updating the plugin didn't. I did that thinking it was a Javadoc issue and just kept the bump. The issue was a Javadoc error caused by a javadoc parameter entry that didn't exist. Eclipse warns about this, but VSCode doesn't. |
david-waltermire
merged commit 6401200
into
metaschema-framework:develop
Committer Notes
This PR adds
fn:substring, 1 of 3 to be added as part of work for #132.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:
Have you updated all website and readme documentation affected by the changes you made?