RDF Messages support added - #586
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds RDF Message (message-log) support: lexer recognizes ChangesRDF Messages Support
sequenceDiagram
participant Client as Client
participant StreamParser as StreamParser
participant N3Parser as N3Parser
participant Writer as Writer
Client->>StreamParser: pipe input (may include VERSION / MESSAGE)
StreamParser->>N3Parser: feed chunks
N3Parser-->>StreamParser: onQuad (emitted) and buffer quad if message mode
alt message delimiter encountered or EOF
N3Parser->>N3Parser: _endMessage() flushes buffered quads
N3Parser-->>StreamParser: onMessage(quads)
StreamParser-->>Client: emit "message" event (quads)
end
Client->>Writer: addMessage(quads)
Writer->>Writer: _writeMessageDelimiter() if not first message
Writer-->>Client: writes quads and `@message .` delimiter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/N3Parser.js (1)
1236-1277:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't require
onQuadjust to receiveonMessage.
onMessageis extracted from the callback object, but Line 1268 still switches to the synchronous path wheneveronQuadis absent.parser.parse(input, { onMessage })therefore never fires the new callback, so consumers that only want grouped messages can't use the new API.Suggested fix
- // Parse synchronously if no quad callback is given - if (!onQuad) { + const hasCallbacks = onQuad || onPrefix || onComment || onVersion || onMessage; + // Parse synchronously only if no callbacks are given at all + if (!hasCallbacks) { const quads = []; let error; this._callback = (e, t) => { e ? (error = e) : t && quads.push(t); }; this._lexer.tokenize(input).every(token => { return this._readCallback = this._readCallback(token); @@ - this._callback = onQuad; + this._callback = onQuad || noop;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/N3Parser.js` around lines 1236 - 1277, The parser currently takes onMessage from the callback object but still forces the synchronous path whenever onQuad is missing; change the synchronous branch condition so it only triggers when neither onQuad nor onMessage is provided (i.e. replace "if (!onQuad)" with "if (!onQuad && !onMessage)"), so that when parse is called with { onMessage } the async/token loop runs and this._messageCallback will be invoked; ensure you keep existing assignments to this._messageCallback and _callback logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/N3Parser.js`:
- Around line 1119-1126: _endMessage currently only resets per-message
blank-node prefix when this._blankNodePrefix is falsy, which allows reuse of the
same blank-node label across messages when blankNodePrefix is configured; modify
_endMessage so that when this._messageMode is true (after emitting message
quads) you always update this._prefixes._ to a unique per-message value (e.g.,
`b${blankNodePrefix++}_`) regardless of this._blankNodePrefix, preserving
blank-node isolation across messages; locate the _endMessage method and replace
the conditional assignment that checks this._blankNodePrefix with an
unconditional assignment under the same this._messageMode guard (using the
existing blankNodePrefix counter).
In `@src/N3Writer.js`:
- Around line 266-273: The addMessage method currently calls done immediately,
before writes triggered by _writeMessageDelimiter and addQuads have been
flushed; change addMessage so done is invoked only after the underlying writes
complete (i.e., after addQuads finishes flushing). Locate addMessage and
coordinate with _writeMessageDelimiter and addQuads: if addQuads returns a
Promise or accepts a callback, await or use its completion callback; otherwise,
use the underlying stream write/drain/finish events (or a write callback) to
detect when all data emitted by _writeMessageDelimiter and addQuads is flushed,
and call done there instead of immediately.
- Around line 276-280: When closing a message in _writeMessageDelimiter(), also
reset the current graph state so the next message won't incorrectly assume the
previous graph is open; after setting this._subject = null add code to set
this._graph = null (or undefined) so _writeQuad will reopen a named graph when
needed (refer to methods _writeMessageDelimiter, _writeQuad and the this._graph
property).
---
Outside diff comments:
In `@src/N3Parser.js`:
- Around line 1236-1277: The parser currently takes onMessage from the callback
object but still forces the synchronous path whenever onQuad is missing; change
the synchronous branch condition so it only triggers when neither onQuad nor
onMessage is provided (i.e. replace "if (!onQuad)" with "if (!onQuad &&
!onMessage)"), so that when parse is called with { onMessage } the async/token
loop runs and this._messageCallback will be invoked; ensure you keep existing
assignments to this._messageCallback and _callback logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66b72a56-c2b6-4c68-81a6-b34c0fc18f46
📒 Files selected for processing (8)
README.mdsrc/N3Lexer.jssrc/N3Parser.jssrc/N3StreamParser.jssrc/N3Writer.jstest/N3Parser-test.jstest/N3StreamParser-test.jstest/N3Writer-test.js
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/N3Writer.js (3)
276-280:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
this._graphnot reset after closing a named-graph block — unresolved from a prior review.After the named-graph close (
\n}\n) andthis._subject = null,this._graphstill holds the previous graph. When the next message's first quad is in that same named graph,_writeQuadwill skip reopening the block, producing malformed output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/N3Writer.js` around lines 276 - 280, The _writeMessageDelimiter method closes a named-graph block but does not reset this._graph, causing subsequent quads to think the graph is still open; update _writeMessageDelimiter (used alongside _writeQuad) to set this._graph = null (in addition to this._subject = null) when closing a named graph so future quads will correctly reopen or create the graph block.
265-273:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
donestill fires before stream writes are flushed — unresolved from a prior review.
addQuads→addQuad→_writeschedules asyncoutputStream.writecalls, butdoneis invoked synchronously at line 272, before any of those writes have completed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/N3Writer.js` around lines 265 - 273, addMessage currently calls done synchronously while addQuads → addQuad → _write schedule asynchronous outputStream.write operations, so done fires before writes flush; change the write completion flow to propagate async completion back to addMessage and invoke done only after the stream has actually flushed. Concretely: make addQuads (and transitively addQuad/_write) return a Promise or accept a callback that resolves/calls when all writes are finished (handle stream.write returning false by awaiting the 'drain' event or using the write callback where supported), then modify addMessage to await that Promise (or use the callback) and call done only after resolution; reference the functions addMessage, addQuads, addQuad, _write and the use of outputStream.write when implementing this propagation.
281-281:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix N-Triples/N-Quads message delimiters to use
MESSAGEkeyword instead of Turtle syntax.When writing N-Triples or N-Quads format (
_lineMode = true), the message delimiter must be theMESSAGEkeyword recognized by the lexer, not the Turtle@message .syntax. The parser's test cases confirm that N-Quads expectsMESSAGEas the delimiter, but the writer currently outputs@message .unconditionally, breaking round-trip serialization.Proposed fix
- this._write('@message .\n'); + this._write(this._lineMode ? 'MESSAGE\n' : '@message .\n');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/N3Writer.js` at line 281, The writer currently emits Turtle-style "@message ." unconditionally; change the logic in N3Writer so that when _lineMode is true (N-Triples/N-Quads output) it writes the lexer-recognized message delimiter "MESSAGE .\n" instead of "@message .\n"—modify the code path using this._write(...) around the _lineMode check in the N3Writer class (the spot calling this._write('@message .\n')) to emit "MESSAGE .\n" for _lineMode true and preserve the existing Turtle "@message .\n" for non-lineMode formats.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/N3Writer.js`:
- Around line 276-280: The _writeMessageDelimiter method closes a named-graph
block but does not reset this._graph, causing subsequent quads to think the
graph is still open; update _writeMessageDelimiter (used alongside _writeQuad)
to set this._graph = null (in addition to this._subject = null) when closing a
named graph so future quads will correctly reopen or create the graph block.
- Around line 265-273: addMessage currently calls done synchronously while
addQuads → addQuad → _write schedule asynchronous outputStream.write operations,
so done fires before writes flush; change the write completion flow to propagate
async completion back to addMessage and invoke done only after the stream has
actually flushed. Concretely: make addQuads (and transitively addQuad/_write)
return a Promise or accept a callback that resolves/calls when all writes are
finished (handle stream.write returning false by awaiting the 'drain' event or
using the write callback where supported), then modify addMessage to await that
Promise (or use the callback) and call done only after resolution; reference the
functions addMessage, addQuads, addQuad, _write and the use of
outputStream.write when implementing this propagation.
- Line 281: The writer currently emits Turtle-style "@message ."
unconditionally; change the logic in N3Writer so that when _lineMode is true
(N-Triples/N-Quads output) it writes the lexer-recognized message delimiter
"MESSAGE .\n" instead of "@message .\n"—modify the code path using
this._write(...) around the _lineMode check in the N3Writer class (the spot
calling this._write('@message .\n')) to emit "MESSAGE .\n" for _lineMode true
and preserve the existing Turtle "@message .\n" for non-lineMode formats.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54cb45bd-77af-4493-a1b0-5316b17489ff
📒 Files selected for processing (4)
README.mdsrc/N3Writer.jstest/N3Parser-test.jstest/N3Writer-test.js
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- test/N3Parser-test.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/N3Writer-test.js (1)
119-134: ⚡ Quick winMake the injected write error less chunk-shape brittle.
Failing only on
chunk === '\n}\n'can break with harmless write batching/splitting changes in the writer implementation.♻️ Suggested hardening
- outputStream = { - write(chunk, encoding, callback) { - callback && callback(chunk === '\n}\n' ? expectedError : undefined); - }, - }, + outputStream = { + _failed: false, + write(chunk, encoding, callback) { + const closesGraph = + typeof chunk === 'string' && (chunk.includes('\n}\n') || chunk === '}\n'); + const error = !this._failed && closesGraph ? expectedError : undefined; + this._failed = this._failed || Boolean(error); + callback && callback(error); + }, + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/N3Writer-test.js` around lines 119 - 134, The injected write error in the test is brittle because it only fires when chunk === '\n}\n'; change the condition in the mocked outputStream.write (used with Writer and addMessage in this test) to detect the RDF message delimiter more robustly (for example use chunk.includes('}\n') or chunk.endsWith('}\n') or a regex test) so the error is triggered whenever the delimiter appears even if writes are batched or split.test/N3Parser-test.js (1)
1121-1133: ⚡ Quick winStrengthen this test to assert configured prefix retention, not just uniqueness.
Right now it only checks that the two blank node IDs differ; it would still pass if the parser stops honoring
blankNodePrefixafter the first message. Add assertions that both subjects keep the configured prefix shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/N3Parser-test.js` around lines 1121 - 1133, The test should not only check uniqueness of blank node IDs but also that the configured blankNodePrefix is retained for each message: update the onQuad/assertion block in the 'should scope blank node labels...' test (using Parser with blankNodePrefix '_:fixed' and the messages array) to assert that both messages[0][0].subject.value and messages[1][0].subject.value begin with or include the configured prefix '_:fixed' in addition to being different; keep the existing uniqueness assertion and add two prefix assertions referencing the same subject values so the parser is validated for prefix retention across RDF messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/N3Parser.js`:
- Around line 1125-1126: The current code in the message flushing path
unconditionally overwrites this._prefixes._ with `b${blankNodePrefix++}_`,
discarding a user-provided blankNodePrefix; change it so we only assign the
autogenerated `b${...}_` when no user prefix exists. Concretely, update the
logic around this._messageMode to check if this._prefixes._ is falsy/undefined
before setting it, or maintain a separate internal counter (e.g.,
internalBlankNodeCounter) used to build `b${...}_` without mutating a configured
blankNodePrefix; references to adjust: this._messageMode, this._prefixes._, and
the blankNodePrefix variable.
---
Nitpick comments:
In `@test/N3Parser-test.js`:
- Around line 1121-1133: The test should not only check uniqueness of blank node
IDs but also that the configured blankNodePrefix is retained for each message:
update the onQuad/assertion block in the 'should scope blank node labels...'
test (using Parser with blankNodePrefix '_:fixed' and the messages array) to
assert that both messages[0][0].subject.value and messages[1][0].subject.value
begin with or include the configured prefix '_:fixed' in addition to being
different; keep the existing uniqueness assertion and add two prefix assertions
referencing the same subject values so the parser is validated for prefix
retention across RDF messages.
In `@test/N3Writer-test.js`:
- Around line 119-134: The injected write error in the test is brittle because
it only fires when chunk === '\n}\n'; change the condition in the mocked
outputStream.write (used with Writer and addMessage in this test) to detect the
RDF message delimiter more robustly (for example use chunk.includes('}\n') or
chunk.endsWith('}\n') or a regex test) so the error is triggered whenever the
delimiter appears even if writes are batched or split.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e564559-d215-4cbd-971d-45d27af4e2d6
📒 Files selected for processing (4)
src/N3Parser.jssrc/N3Writer.jstest/N3Parser-test.jstest/N3Writer-test.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/N3Parser.js`:
- Around line 822-825: The parser sets this._messageMode inside token handling,
which can leak state across multiple parse() calls; to fix, reset
this._messageMode (and if needed call _resetMessageBlankNodePrefix()) at the
start of the parse() method so each parse run starts with a clean message-mode
flag; update the parse() implementation to explicitly set this._messageMode =
false (and invoke _resetMessageBlankNodePrefix() if blank-node prefix must be
cleared) before processing input to avoid cross-run state leakage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 189991cd-8f45-435b-8c33-8aff5e6f51f7
📒 Files selected for processing (2)
src/N3Parser.jstest/N3Parser-test.js
|
I believe there are no remaining issues here and this PR is ready to be checked by the maintainers. |
|
I'd propose to only add features to N3.js main that are W3C recommendations. Could this be implemented as a subclass of the Lexer and Parser, in a separate module? |
It’s our ambition that RDF Messages ends up in a W3C recommendation: either the core RDF one, either the RDF Stream Processing (RSP) working group that is being created. It’s however a chicken-egg situation: for it to move on we need implementations in important frameworks. The rule could be perceived as confusing: N3 itself is also not a recommendation, and RDF1.2 triple terms are also not yet a recommendation (of course the adoption in this library benefits the upcoming recommendation). We are now promoting this work to the broader community now that the RDF Messages proposal has reached a decent draft state in the W3C RSP CG. Next week we’re promoting it at the ESWC conference.
There are 100 additional lines of code we propose here in the src folder. After building this for the browser, when I build Emotional argument: the RDF Messages proposal is worth championing as it should be close to your heart: it’s a basis for message stream logs that can provide a provenance trail for trustworthy data flows, where I see trust envelopes as a fundamental next piece of the puzzle. |
2 participants
This is an implementation of the RDF Messages specification by the W3C RDF Stream Processing CG that is now seeking for implementations of the spec.
More context:
Summary by CodeRabbit
New Features
Behavior
Tests
Documentation