Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ntj/nuo db data reader #37

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Ntj/nuo db data reader #37

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7449ce0
Implement NuoDbDataReader.HasRows()
May 6, 2024
f44ae50
Add further safety check in Read().
May 21, 2024
7841f91
Fix typo in comment.
May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions NuoDb.Data.Client/NuoDbDataReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ private void InitResultSet(int handle, EncodedDataStream dataStream, bool readCo
this.values = new Value[numberColumns];
this.closed = false;
this.currentRow = 0;
this.afterLast = false;
// this.afterLast = false;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why leave code commented out?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was in a block that previously set ALL the member fields.

I agree that leaving a commented line is poor style.
I'll replace the commented line with a comment that explains why afterLast is not being set now.

The alternative was to set afterLast = false here, as originally done, and then set its value later.
But, I thought was was potentially confusing.

this.declaredColumnTypes = null;
this.declaredColumnTypeNames = null;

Expand All @@ -93,6 +93,9 @@ private void InitResultSet(int handle, EncodedDataStream dataStream, bool readCo
//RemPreparedStatement ps = (RemPreparedStatement)statement;
//columnNames = ps.columnNames;
}

// Set afterLast to true if the ResultSet is empty.
this.afterLast = (this.pendingRows == null || this.pendingRows.getInt() == 0);
}

protected override void Dispose(bool disposing)
Expand Down Expand Up @@ -307,14 +310,17 @@ public override bool NextResult()

public override bool Read()
{
if (this.pendingRows == null)
// Currently, afterLast can only be false if pendingRows was non-null in InitResultSet();
// - but InitResultSet could be changed in the future.
if (afterLast || pendingRows == null)
return false;

//int maxRows = statement == null ? 0 : statement.MaxRows;
int maxRows = 0;
int maxRows = 0; // this local maxRows HIDES this.maxRows
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this variable is hiding a class field, why not rename it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I regard this as a hack,

It was in the original code - I didn't make this change originally.

I left this highly dubious code for now, because I wanted just to implement the needed feature in this PR - and not get embroiled in cleaning this code up and ensuring the correct behaviour remains.

In a later PR, I will fix this code by removing the hiding, and cleaning up the code that relies on the hidden var.

To answer you question, there is a condition later in this method that tests maxRows, so hiding maxRows with a local zero (0) value effectively disables that test.
Which is the intention of this code.

I think the long term solution is to just make sure this class works correctly in the absence of a reliable maxRows value.


for (; ; )
{
// NOTE: this is a LOCAL maxRows which is hiding this.maxRows.
if (maxRows > 0 && currentRow >= maxRows)
{
afterLast = true;
Expand All @@ -324,7 +330,8 @@ public override bool Read()

if (!pendingRows.EndOfMessage)
{
int result = pendingRows.getInt();
// InitResultSet() performs the pendingRows.getInt() for currentRow == 0
int result = currentRow > 0 ? pendingRows.getInt() : -1;

if (result == 0)
{
Expand Down Expand Up @@ -653,7 +660,7 @@ public override System.Collections.IEnumerator GetEnumerator()

public override bool HasRows
{
get { throw new NotImplementedException(); }
get { return (!afterLast); }
}
}
}