-
Notifications
You must be signed in to change notification settings - Fork 0
Fix duplicate handle field and data accumulation in JSON continue responses #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: candidate-9.14.x
Are you sure you want to change the base?
Fix duplicate handle field and data accumulation in JSON continue responses #143
Conversation
Co-authored-by: jakesmith <902700+jakesmith@users.noreply.github.com>
jakesmith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot - add a new commit which adds some basic (not too verbose) CPPUNIT tests in this file, that include testing continuation with json. To do so you will need functionality like testStartServer(), and to clearup the created 'serverThread' at the end. Extract this functionaliy out of the existing RemoteFileSlowTest suite, so it can be reused by the new one too.
Co-authored-by: jakesmith <902700+jakesmith@users.noreply.github.com>
Added JsonStreamingTest suite with basic JSON continuation tests in commit 023ab1e. Extracted testStartServer() and testStopServer() functionality from RemoteFileSlowTest into reusable static functions that both test suites can use. The new test suite includes:
The extracted server functions include CServerThread class and handle port selection, server startup, and cleanup automatically. |
jakesmith
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot - the unittest is insufficient. It must actually connect to the rowservice, and test the protocol
…mmunication Co-authored-by: jakesmith <902700+jakesmith@users.noreply.github.com>
Enhanced the unit tests in commit 3ee256a to actually connect to the row service and test the JSON streaming protocol. The tests now: testJsonStreamingBasic():
testJsonContinuation():
Both tests perform actual socket communication, send properly formatted protocol messages, and parse JSON responses to verify the fix works correctly. |
Problem
The row-service continue request with JSON format had multiple critical issues that made it unusable:
{"handle": 32, "handle": 32, ...}which violates JSON specificationExample of the Issue
First request (newstream):
{ "Response": { "handle": 32, "handle": 32, "Row": [/* 4 rows */], "cursorBin": "AAAAAAAAAaYAAAAAAAAABAEA" } }Continue request (broken):
{ "Response": { "handle": 32, "handle": 32, "Row": [/* SAME 4 rows as before */], "cursorBin": "AAAAAAAAAaYAAAAAAAAABAEA" } }, "handle": 32, "Row": [/* 1 new row */], "cursorBin": "AAAAAAAAAhIAAAAAAAAABQEA" }Root Cause
The
CRemoteRequestclass initialized theresponseWriterin its constructor, which wrote the initial JSON structure including the "handle" field. For continue requests, the sameCRemoteRequestobject was reused, and theprocess()method would output another "handle" field, creating duplicates and accumulating data from previous requests.Solution
Moved the
responseWriterinitialization from the constructor to theprocess()method. This ensures:Test Coverage Added
Added comprehensive
JsonStreamingTestCPPUNIT test suite with:testStartServer()andtestStopServer()fromRemoteFileSlowTestinto reusable static functions for shared use across test suitesThe tests perform real communication with the dafilesrv using proper protocol messages and CSV test data to ensure the fix works in realistic scenarios.
Expected Results After Fix
First request:
{ "Response": { "handle": 32, "Row": [/* 4 rows */], "cursorBin": "AAAAAAAAAaYAAAAAAAAABAEA" } }Continue request:
{ "Response": { "handle": 32, "Row": [/* 1 NEW row only */], "cursorBin": "AAAAAAAAAhIAAAAAAAAABQEA" } }Backward Compatibility
The changes maintain full backward compatibility:
This fix makes the JSON continue functionality actually usable for the first time, resolving the invalid JSON syntax and incorrect data accumulation that prevented proper client-side parsing and processing.
Original prompt
The row-service (implemented in dafserver.cpp) continue request with json format does not work as expected, and needs to be fixed.
In the example below the replyLimit is limited to 1, but the conclusions are the same for larger datasets.
The continue response seems to return the same data as the original response with one additional row appended.
The handle is also repeated multiple times in both responses.
Example:
File: gravesee::test::titanic
First request:
{'command': 'newstream',
'format': 'json',
'node': {'filePart': 1,
'filePartCopy': 1,
'input': {'fieldType': 1037,
'fields': [{'flags': 1, 'name': 'survived', 'type': 'ty1'},
{'flags': 1, 'name': 'pclass', 'type': 'ty1'},
{'flags': 1065, 'name': 'sex', 'type': 'ty2'},
{'flags': 2, 'name': 'age', 'type': 'ty3'},
{'flags': 1, 'name': 'sibsp', 'type': 'ty1'},
{'flags': 1, 'name': 'parch', 'type': 'ty1'},
{'flags': 2, 'name': 'fare', 'type': 'ty3'},
{'flags': 1065,
'name': 'embarked',
'type': 'ty2'},
{'flags': 1065, 'name': 'class', 'type': 'ty2'},
{'flags': 1065, 'name': 'who', 'type': 'ty2'},
{'name': 'adult_male', 'type': 'ty4'},
{'flags': 1065, 'name': 'deck', 'type': 'ty2'},
{'flags': 1065,
'name': 'embark_town',
'type': 'ty2'},
{'flags': 1065, 'name': 'alive', 'type': 'ty2'},
{'name': 'alone', 'type': 'ty4'}],
'length': 78,
'ty1': {'fieldType': 1, 'length': 8},
'ty2': {'fieldType': 1065, 'length': 4294967281},
'ty3': {'fieldType': 2, 'length': 8},
'ty4': {'fieldType': 0, 'length': 1}},
'metaInfo': REDACTED,
'output': {'fieldType': 1037,
'fields': [{'flags': 1, 'name': 'survived', 'type': 'ty1'},
{'flags': 1, 'name': 'pclass', 'type': 'ty1'},
{'flags': 1065, 'name': 'sex', 'type': 'ty2'},
{'flags': 2, 'name': 'age', 'type': 'ty3'},
{'flags': 1, 'name': 'sibsp', 'type': 'ty1'},
{'flags': 1, 'name': 'parch', 'type': 'ty1'},
{'flags': 2, 'name': 'fare', 'type': 'ty3'},
{'flags': 1065,
'name': 'embarked',
'type': 'ty2'},
{'flags': 1065, 'name': 'class', 'type': 'ty2'},
{'flags': 1065, 'name': 'who', 'type': 'ty2'},
{'name': 'adult_male', 'type': 'ty4'},
{'flags': 1065, 'name': 'deck', 'type': 'ty2'},
{'flags': 1065,
'name': 'embark_town',
'type': 'ty2'},
{'flags': 1065, 'name': 'alive', 'type': 'ty2'},
{'name': 'alone', 'type': 'ty4'}],
'length': 78,
'ty1': {'fieldType': 1, 'length': 8},
'ty2': {'fieldType': 1065, 'length': 4294967281},
'ty3': {'fieldType': 2, 'length': 8},
'ty4': {'fieldType': 0, 'length': 1}}},
'replyLimit': 1}
Second request:
{'format': 'json', 'handle': 32, 'command': 'continue'}
RESPONSES:
First Response:
{
"Response": {
"handle": 32,
"handle": 32,
"Row": [
{
"survived": "0",
"pclass": "2",
"sex": "male",
"age": 42.0,
"sibsp": "0",
"parch": "0",
"fare": 13.0,
"embarked": "S",
"class": "Second",
"who": "man",
"adult_male": true,
"deck": "NaN",
"embark_town": "Southampton",
"alive": "no",
"alone": true
},
{
"survived": "0",
"pclass": "3",
"sex": "male",
"age": 28.0,
"sibsp": "0",
"parch": "0",
"fare": 7.8958,
"embarked": "S",
"class": "Third",
"who": "man",
"adult_male": true,
"deck": "NaN",
"embark_town": "Southampton",
"alive": "no",
"alone": true
},
{
"survived": "1",
"pclass": "1",
"sex": "female",
"age": 38.0,
"sibsp": "0",
"parch": "0",
"fare": 80.0,
"embarked...
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.