Fix Seek Operations in SftpFileStream

[lemonyte]

Changed SftpFileStream.Seek() to add offset instead of subtracting when origin is set to SeekOrigin.End. Fixes #909, fixes #1018.

[IgorMilavec]

Good catch!

Can you also cover cases for invalid origin and trying to move before the beginning of the file? Here are the exceptions I feel would better describe the problems (and are somewhat aligned with the FileStream behavior):

throw new ArgumentException("Invalid seek origin.", "origin")
throw new IOException("An attempt was made to move the file pointer before the beginning of the file.");
throw new EndOfStreamException("An attempt was made to move the file pointer past the end of the file.");

I see that you have changed formatting of the code. Based on my past experience the maintainer will request you to undo these changes.

Can you add tests for the cases you mentioned were not covered?

[lemonyte]

I see that you have changed formatting of the code.

I am not sure what formatting changes you are referring to. The only change other than replacing - with + was the removal of a duplicate identical code block, as below:

if (condition)
{
    // Condition specific code

    switch (origin)
    {
        // ...
    }

    if (newPosn == -1)
    {
        throw new EndOfStreamException("End of stream.");
    }
    _position = newPosn;
}
else
{
    // Condition specific code

    switch (origin)
    {
        // ...
    }

    if (newPosn < 0)
    {
        throw new EndOfStreamException();
    }
    _position = newPosn;
}

Refactored to:

if (condition)
{
    // Condition specific code
}
else
{
    // Condition specific code
}

switch (origin)
{
    // ...
}
if (newPosn < 0)
{
    throw new EndOfStreamException("End of stream.");
}
_position = newPosn;

throw new EndOfStreamException("An attempt was made to move the file pointer past the end of the file.");

I think a seek operation past the end of a file is a valid operation, but correct me if I am wrong. Most platforms allow seeking past the end of a file, only raising an exception when a read is attempted after EOF. A case for seeking before the beginning of a file already exists:

SSH.NET/src/Renci.SshNet/Sftp/SftpFileStream.cs

Line 808 in 7bdfc9e

throw new EndOfStreamException("End of stream.");

SSH.NET/src/Renci.SshNet/Sftp/SftpFileStream.cs

Line 859 in 7bdfc9e

throw new EndOfStreamException();

Can you add tests for the cases you mentioned were not covered?

I am not very experienced in C#, being primarily a Python developer, but I'll see what I can reverse-engineer from the existing tests. Some guidance on how to use the available tests would be much appreciated.

[IgorMilavec]

To run the tests in VS 2019, open Test Explorer and you will see a list of tests. Filter Traits (there is a filter icon at the column heading) and deselect "integration" and "LongRunning". Then run all the tests from Test Explorer.

To cover this scenario, you create new test classes SftpFileStreamTest_Seek_PositionedAtBeginningOfStream_OriginEndAndOffset* by copiing SftpFileStreamTest_Seek_PositionedAtBeginningOfStream_OriginBeginAndOffset* and adjusting appropriately.

Also, due to the changes in this PR, the SeekShouldHaveThrownEndOfStreamException test is failing, I will flag the probable cause for this in the code review.

[IgorMilavec]

You are providing a non-default exception message, causing the test SeekShouldHaveThrownEndOfStreamException to fail. I beleive if you just use throw new EndOfStreamException(); this will be fixed.

[lemonyte]

Thanks for pointing that out. The exception message is already present in another part of the code, so I assumed I should leave it in there.

[IgorMilavec]

@drieseng There have been suggestions in other issues that we should localize exception messages. While that is probably too much work at this time, I propose to use default exception messages where possible and to stop checking error messages in the tests and rely on only asserting exception type (and other structured info in the exception) in the tests. Do you agree?

[lemonyte]

I have noticed that the exception message format is different across .NET versions. For example:
.NET Framework 3.5

ArgumentException error message.
Parameter name: ParamName

.NET Core 3.0

ArgumentException error message. (Parameter 'ParamName')

This causes some tests to pass on one framework and fail on another.

[drieseng]

@IgorMilavec, in general I prefer very specific asserts. To be honest, I think we have more important stuff to deal with than localization. But I'm ok to discuss this as a team, and decide where we want to go with this.

[drieseng]

@LemonPi314, that's indeed something I was already aware of and which I dealt with in other (professional) projects.

[IgorMilavec]

Changed SftpFileStream.Seek() to add offset instead of subtracting when origin is set to SeekOrigin.End. (#909)

@LemonPi314 can you please edit the above to include "Fix #909" so this PR will be linked to the issue? Tnx!

[daviburg]

Test case for the error(s) been fixed is still needed in this PR.

[daviburg]

Can you also cover cases for invalid origin and trying to move before the beginning of the file? Here are the exceptions I feel would better describe the problems (and are somewhat aligned with the FileStream behavior):

throw new ArgumentException("Invalid seek origin.", "origin")
throw new IOException("An attempt was made to move the file pointer before the beginning of the file.");
throw new EndOfStreamException("An attempt was made to move the file pointer past the end of the file.");

I second this suggestion to use specific and descriptive exceptions.

[daviburg]

I see that you have changed formatting of the code. Based on my past experience the maintainer will request you to undo these changes.

I am ambivalent about that. If the formatting change is specific to the code been already updated for the fix, and covered by the tests, I find it a good opportunity to clean up.
If the formatting change is extensive and makes it difficult to understand the functional change, I can see how that may best be done in a separate PR.

[drieseng]

I am ambivalent about that. If the formatting change is specific to the code been already updated for the fix, and covered by the tests, I find it a good opportunity to clean up. If the formatting change is extensive and makes it difficult to understand the functional change, I can see how that may best be done in a separate PR.

In general, I prefer a separate PR unless we're talking about a very minimal change.
To avoid discussions on what such a minimal change is, it may be better to not mix formatting and non-formatting changes at all.

[lemonyte]

I've added tests for seeking from SeekOrigin.End.
Regarding formatting changes, to be frank I do not see what formatting exactly is causing an issue, but I will keep your feedback in mind for future contributions.
As for exceptions and exception messages, those are out of the scope of this PR, and would best be addressed separately in a new PR.

I do not want to pull this thread any more off-topic than it already is, but I think it is worth noting that while developing this PR I've encountered several issues with the tests. Notably, failures in tests unrelated to the changed portions (see AppVeyor checks 45419708 and 45419751), and failures caused by exception messages not matching (see below and this comment).

 Ctor_PartialSuccessLimit_Zero
   Source: ClientAuthenticationTest.cs line 19
   Duration: < 1 ms

  Message: 
    Assert.AreEqual failed. Expected:<Cannot be less than one.
    Parameter name: partialSuccessLimit>. Actual:<Cannot be less than one. (Parameter 'partialSuccessLimit')>. 

  Stack Trace: 
    ClientAuthenticationTest.Ctor_PartialSuccessLimit_Zero() line 31

If there is anything else I can do to improve this PR, let me know. In any case, hopefully this fix is merged soon.

[drieseng]

@LemonPi314, your changes are looking good. Thanks!

I'd only like to hold off on merging until I (or someone else) had time to add a few integration tests for this.
Should this take too long, do not hesitate to spam me :-)

[drieseng]

I'll submit a PR for the integration test changes once sshnet/IntegrationTests#8 has landed.

[drieseng]

Please add leading forward slash since this ignore only applies to the root directory of the repo.

[drieseng]

@IgorMilavec, in general I prefer very specific asserts. To be honest, I think we have more important stuff to deal with than localization. But I'm ok to discuss this as a team, and decide where we want to go with this.

[drieseng]

@LemonPi314, that's indeed something I was already aware of and which I dealt with in other (professional) projects.