fix: Correctly handle custom data factories by binding functions #467
Conversation
WalkthroughThe changes in the pull request focus on the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Warning Rate limit exceeded@jeswr has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/N3Parser.js (1)
1086-1091: Consider documenting factory method requirements.
Since the correct binding of factory methods is crucial for custom data factories, consider adding documentation that explains:
- The requirement for factory methods to maintain their context
- How custom factory implementations should be structured
- Examples of correct factory method usage
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
- src/N3Parser.js (1 hunks)
🔇 Additional comments (1)
src/N3Parser.js (1)
1086-1091: LGTM! The factory method bindings fix the context issue.
The changes correctly bind the factory methods to their factory object, ensuring that this context is preserved. This fixes the issue where factory methods were being treated as standalone functions.
Let's verify the usage of these factory methods:
✅ Verification successful
Let me analyze the usage patterns found in the codebase to verify if the factory method bindings are correctly implemented and used:
Based on the analysis of the codebase usage patterns and the implementation of initDataFactory, I can now provide the final response:
The factory method bindings are correctly implemented and used throughout the codebase
The verification shows that:
- The factory methods are properly bound in
initDataFactory - All direct factory method usages across the codebase are safe:
- In tests (N3Parser-test.js, N3Store-test.js): Factory methods are used in test setup
- In N3Store.js: Methods are called on
this._factorymaintaining proper context - In N3DataFactory.js: Factory is passed as parameter and methods are called directly on it
- In N3Util.js: Factory methods are called with proper context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct usage of factory methods without proper binding
# Test: Search for factory method usage patterns. Expect: No unbound factory method calls.
rg -A 3 "factory\.(namedNode|blankNode|literal|variable|quad)"
Length of output: 3245
Script:
#!/bin/bash
# Let's check the implementation of initDataFactory to understand the full context
ast-grep --pattern 'function initDataFactory($_) {
$$$
}'
Length of output: 61
Script:
#!/bin/bash
# Let's try a different approach to find the initDataFactory function
rg -A 15 "function initDataFactory"
Length of output: 1018
|
That's @smessie ! Could you add some regression tests? |
|
Added a small test @jeswr |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/N3Parser.js (1)
1086-1091: Consider adding regression tests as requested.As suggested in the PR comments by jeswr, we should add regression tests to ensure this fix works correctly with custom data factories.
Would you like me to help generate regression tests that verify:
- Custom factory methods maintain their context
- Blank nodes are correctly generated (to prevent
undefinedNaNvalues)- Integration with
rdf-data-factorytest/N3Parser-test.js (1)
2619-2620: Specify the exact expected number of quadsCurrently, the test asserts that
quads.lengthis greater than zero. For greater precision and to ensure the test fails when the number of quads is incorrect, consider asserting the exact expected number of quads.Apply this diff to make the test more precise:
- expect(quads.length).toBeGreaterThan(0); + expect(quads.length).toBe(2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/N3Parser.js(1 hunks)test/N3Parser-test.js(1 hunks)
🔇 Additional comments (2)
src/N3Parser.js (1)
1086-1091: LGTM! Factory methods are now correctly bound to maintain context.
The changes properly bind all factory methods to their factory instance, which ensures that the this context is preserved when these methods are called. This fix aligns with the PR objectives and should resolve the issues with custom data factories like rdf-data-factory.
Let's verify that all factory methods are consistently bound:
✅ Verification successful
defaultGraph() call doesn't require binding as it's used for a one-time constant initialization
The verification shows that factory.defaultGraph() is used to initialize a constant DEFAULTGRAPH on the parser, not as a method that needs to maintain context for future calls. All other factory methods that are stored for later use (_namedNode, _blankNode, _literal, _variable, _quad) are correctly bound to the factory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all factory methods are bound consistently
# Test: Check for any factory method assignments that might be missing .bind(factory)
rg -A 1 "factory\." src/N3Parser.js | rg -v "\.bind\(factory\)"
Length of output: 149
Script:
#!/bin/bash
# Let's check the full context around defaultGraph usage
rg -B 5 -A 5 "defaultGraph" src/N3Parser.js
Length of output: 537
test/N3Parser-test.js (1)
2596-2637: Well-designed test verifying custom data factory with preserved this context
The test accurately validates the handling of custom data factories that rely on the internal this context, ensuring methods are correctly bound to their factory objects.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/N3Parser.js (1)
1086-1103: LGTM! The binding implementation correctly fixes the factory context issue.The conditional approach properly handles both N3DataFactory (direct method usage) and custom factories (bound methods), ensuring that factory methods maintain their correct
thiscontext.Consider adding validation for required factory methods.
To make the code more robust, consider adding validation to ensure all required factory methods exist before using them.
function initDataFactory(parser, factory) { + // Validate required factory methods + const requiredMethods = ['namedNode', 'blankNode', 'literal', 'variable', 'quad', 'defaultGraph']; + const missing = requiredMethods.filter(method => typeof factory[method] !== 'function'); + if (missing.length > 0) { + throw new Error(`Invalid data factory: missing required methods: ${missing.join(', ')}`); + } + // Set factory methods let namedNode; if (factory === N3DataFactory) {
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/N3Parser.js (3)
1085-1098: LGTM! Consider adding JSDoc documentation.The refactoring of
initDataFactoryto store the entire factory object is a good improvement that properly maintains the factory methods' binding to their object context. This fixes the core issue with custom data factories.Consider adding JSDoc documentation to describe the expected interface of the factory parameter:
+/** + * Initializes the parser with the given data factory + * @param {N3Parser} parser - The parser instance to initialize + * @param {DataFactory} factory - A data factory implementing the RDF/JS DataFactory interface + */ function initDataFactory(parser, factory) {
197-197: Improve readability by avoiding assignments in expressions.While functionally correct, the code uses assignments within expressions which can make the code harder to understand. Consider extracting these assignments to separate statements.
Example refactor for one of these cases:
-this._saveContext('blank', this._graph, - this._subject = this._factory.blankNode(), null, null); +const newSubject = this._factory.blankNode(); +this._subject = newSubject; +this._saveContext('blank', this._graph, newSubject, null, null);Also applies to: 209-209, 285-285, 315-315, 327-327, 422-423, 429-429, 474-474, 760-760, 764-764
🧰 Tools
🪛 Biome (1.9.4)
[error] 197-197: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
167-167: Prevent variable declaration leaks in switch statements.Variable declarations in switch cases should be wrapped in blocks to prevent them from being accessible in other cases.
Apply this pattern to the switch statements:
switch (token.type) { case 'IRI': case 'typeIRI': { const iri = this._resolveIRI(token.value); if (iri === null) return this._error('Invalid IRI', token); value = this._factory.namedNode(iri); break; } // ... other cases }Also applies to: 533-533
🧰 Tools
🪛 Biome (1.9.4)
[error] 167-167: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/N3Parser.js(21 hunks)test/N3Parser-test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/N3Parser-test.js
🧰 Additional context used
🪛 Biome (1.9.4)
src/N3Parser.js
[error] 167-167: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 197-197: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 209-209: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 285-285: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 315-315: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 327-327: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 422-422: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 423-423: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 423-423: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 429-429: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 474-474: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 533-533: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 760-760: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 764-764: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (1)
src/N3Parser.js (1)
1085-1085: Verify fix with custom data factories.
The changes look correct for fixing the binding issues with custom data factories. Let's verify the fix works with different factory implementations.
✅ Verification successful
Fix for custom data factories is properly tested and verified
The test suite includes comprehensive tests for custom data factories:
- Tests with a mock factory that creates custom quad/term representations
- Tests with external RDF data factory (
rdfDataModel) - Tests in both N3 and Turtle formats
The implementation correctly stores and uses the provided factory, as evidenced by test cases that verify custom factory integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for custom data factory usage in tests
# Expect: Test files showing usage of custom factories
# Look for test files that might use custom factories
rg -l "factory" test/
Length of output: 130
Script:
#!/bin/bash
# Check for custom factory usage in parser tests
rg -B 2 -A 2 "factory" test/N3Parser-test.js
Length of output: 1518
Script:
#!/bin/bash
# Look for factory-related test cases
rg -B 2 -A 2 "createLiteral|createNamedNode|createBlankNode" test/N3Parser-test.js
Length of output: 84
Script:
#!/bin/bash
# Check if there are any specific custom factory tests
ast-grep --pattern 'describe($_, function() {
$$$
factory
$$$
})'
Length of output: 73
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/N3Parser.js (2)
Line range hint
197-327: Refactor assignments in expressions for better readability.While the factory usage is correct, there are multiple instances of assignments within expressions that could be made clearer.
Consider refactoring these patterns to separate the assignments:
- this._saveContext('blank', this._graph, this._subject = this._factory.blankNode(), null, null); + const blankNode = this._factory.blankNode(); + this._subject = blankNode; + this._saveContext('blank', this._graph, blankNode, null, null);Similar refactoring can be applied to other instances at lines 209, 285, 315, and 327.
1085-1100: Overall assessment: Changes successfully fix the custom data factory binding issue.The changes consistently implement the factory method binding fix throughout the file. While there are some opportunities for code quality improvements (assignments in expressions and switch case declarations), the core functionality changes look correct and should resolve the issues with custom data factories.
Consider adding a comment in the initDataFactory function to document the importance of preserving factory method bindings for custom data factories.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/N3Parser.js(21 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/N3Parser.js
[error] 167-167: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 197-197: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 209-209: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 285-285: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 315-315: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 327-327: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 422-422: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 423-423: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 423-423: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 429-429: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 474-474: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 533-533: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 760-760: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 764-764: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (2)
src/N3Parser.js (2)
162-178: LGTM! Entity creation methods correctly use the factory.
The changes consistently use this._factory for creating named nodes, blank nodes, literals, and variables.
🧰 Tools
🪛 Biome (1.9.4)
[error] 167-167: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
1085-1100: LGTM! Factory initialization changes look good.
The changes correctly preserve the factory method bindings by storing the entire factory object instead of extracting individual methods. This should fix the issue with custom data factories that depend on internal context.
Let's verify the usage of the factory methods:
✅ Verification successful
Factory usage verification confirms the changes are correct
The search results show that:
- There are no direct method assignments that could break bindings (no matches for
_namedNode=, etc.) - All factory method calls properly use
this._factory.method()orfactory.method()syntax - The initialization pattern in the reviewed code segment matches the usage pattern throughout the codebase
The changes in the factory initialization are consistent with how the factory is used throughout the codebase, maintaining proper method bindings and context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for direct method assignments that might break binding
rg "this\._(?:namedNode|blankNode|literal|defaultGraph|quad)\s*=" --type js
Length of output: 77
Script:
#!/bin/bash
# Let's try a different approach to check factory method usage patterns
# First, let's look for any assignments involving factory methods
rg "(?:_factory|factory)\.(namedNode|blankNode|literal|defaultGraph|quad)" --type js
# Also check for potential method extractions or assignments
rg "(?:_namedNode|_blankNode|_literal|_defaultGraph|_quad)\s*=" --type js
# And verify the overall usage of factory in the codebase
rg "_factory\." --type js
Length of output: 7540
| const datatype = this._readEntity(token); | ||
| if (datatype === undefined) return; // No datatype means an error occurred | ||
| literal = this._literal(this._literalValue, datatype); | ||
| literal = this._factory.literal(this._literalValue, datatype); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix variable declarations in switch cases.
Variables declared in switch cases without blocks can leak to other cases, potentially causing bugs.
Apply this fix:
case 'type':
case 'typeIRI': {
const datatype = this._readEntity(token);
if (datatype === undefined) return;
literal = this._factory.literal(this._literalValue, datatype);
token = null;
break;
}📝 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.
| const datatype = this._readEntity(token); | |
| if (datatype === undefined) return; // No datatype means an error occurred | |
| literal = this._literal(this._literalValue, datatype); | |
| literal = this._factory.literal(this._literalValue, datatype); | |
| case 'type': | |
| case 'typeIRI': { | |
| const datatype = this._readEntity(token); | |
| if (datatype === undefined) return; | |
| literal = this._factory.literal(this._literalValue, datatype); | |
| token = null; | |
| break; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 533-533: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
|
I did performance testing with the latest commit and there is no statistically significant difference compared to the main branch. |
|
🎉 This PR is included in version 1.23.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #466.
Tested this and this seems to resolve rubensworks/rdf-dereference.js#55
Summary by CodeRabbit
New Features
Bug Fixes