Remove unnecessary SftpFileStream unit tests and dedup Open{Async} #1680
Conversation
| _handle = _session.RequestOpen(path, flags | Flags.Truncate, nullOnError: true); | ||
| if (_handle is null) | ||
| { | ||
| flags |= Flags.CreateNew; | ||
| } | ||
| else | ||
| { | ||
| flags |= Flags.Truncate; | ||
| } |
There was a problem hiding this comment.
It is at best ineffectual and at worst a bug that we send SSH_FXF_TRUNC without SSH_FXF_CREAT (what we have as Flags.CreateNewOrOpen). Per the draft SFTPv3 spec (https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-02#section-6.3), SSH_FXF_TRUNC must be accompanied by SSH_FXF_CREAT. OpenAsync had the logic correct, so we use that one. I am wondering whether it has something to do with #1215
| try | ||
| else | ||
| { | ||
| var attributes = await session.RequestFStatAsync(handle, cancellationToken).ConfigureAwait(false); | ||
| position = attributes.Size; | ||
| attributes = session.RequestFStat(handle, nullOnError: false); | ||
| } | ||
| catch | ||
| { | ||
| try | ||
| { | ||
| await session.RequestCloseAsync(handle, cancellationToken).ConfigureAwait(false); | ||
| } | ||
| catch | ||
| { | ||
| // The original exception is presumably more informative, so we just ignore this one. | ||
| } |
There was a problem hiding this comment.
the only other library change aside from my comment above is that we use the Open (not OpenAsync) version of the FileMode.Append handling, which doesn't try RequestClose on FStat failure. Seems sensible to call it, but given the sync method is probably used much more and there are seemingly no known issues (with this part), I forwent the RequestClose
# Conflicts: # src/Renci.SshNet/ISubsystemSession.cs
2 participants
SftpFileStream has decent coverage in the integration tests (namely https://github.com/sshnet/SSH.NET/blob/d40bc43ac16d0ba54aae965d4923608dfb5fa475/test/Renci.SshNet.IntegrationTests/SftpTests.cs). It means the unit tests on SftpFileStream are largely redundant, and because they are quite verbose and strongly tied to internal behaviour, serve only to hinder new development. This change deletes a lot of the unit tests; adds a few not explicitly covered by the integration tests; bolsters the integration tests a bit; and finally, deduplicates SftpFileStream.Open/OpenAsync.