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.

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

adds fallback to posix-rename@openssh.com extension if possible and c… #827

Merged
merged 15 commits into from
Oct 23, 2023

Conversation

klelifo
Copy link
Contributor

@klelifo klelifo commented Oct 27, 2022

adds fallback to posix-rename@openssh.com extension if possible and communicates possible problems with flags to the developer

Tested with openssh-sftp-server-1:8.9p1-3 (Ubuntu 22.04)

…ommunicates possible problems with flags to the developer
@sonatype-lift
Copy link

sonatype-lift bot commented Oct 27, 2022

⚠️ 14 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

// build and send request
final Request request = newRequest(type);

if (serverExtension != null)
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add { around the if-statement... Everywhere where that's not done is a leftover ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, i just pushed a corrected version.
Thanks for your quick reply.

@hierynomus
Copy link
Owner

Maybe only thing missing is some (unit) tests

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2022

Codecov Report

Merging #827 (4205bb7) into master (542bb35) will increase coverage by 0.14%.
The diff coverage is 51.42%.

@@             Coverage Diff              @@
##             master     #827      +/-   ##
============================================
+ Coverage     68.40%   68.55%   +0.14%     
- Complexity     1398     1412      +14     
============================================
  Files           207      207              
  Lines          7478     7504      +26     
  Branches        631      642      +11     
============================================
+ Hits           5115     5144      +29     
+ Misses         2025     2015      -10     
- Partials        338      345       +7     
Files Coverage Δ
...rc/main/java/net/schmizz/sshj/sftp/SFTPEngine.java 60.00% <51.42%> (+3.69%) ⬆️

... and 5 files with indirect coverage changes

@klelifo
Copy link
Contributor Author

klelifo commented Nov 3, 2022

I added some simple tests to cover sftp.rename().
Hope this improves things, although I did not add any advanced tests that simulate the behaviour of OpenSSH at the server side to trigger the use of the posix-rename@openssh.com extension.
Not sure how to implement this...

@hierynomus
Copy link
Owner

The integration tests run against an OpenSSH server, so that should be testable.

@klelifo
Copy link
Contributor Author

klelifo commented Aug 4, 2023

Sorry, I don't have much time at the moment.
Just wanted to let you know that I'm still interested in getting this merged.
Will provide the test cases as soon as I find the time.

@hierynomus
Copy link
Owner

The integration tests have been simplified significantly I think. It should not be hard to add a few tests to cover this behaviour now.

@klelifo
Copy link
Contributor Author

klelifo commented Oct 20, 2023

I tried to make the tests as future-proof as possible by allowing to request a specific SFTP protocol version. This way things will not break/become non-functional when sshj will support SFTPv5 or greater in the future.
As of now (MAX_SUPPORTED_VERSION = 3) the implemented fallback for rename flags will always be active anyway - I think.

Copy link
Owner

@hierynomus hierynomus left a comment

Choose a reason for hiding this comment

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

One small nitpick to prevent too many externally visible methods, otherwise great work!

src/main/java/net/schmizz/sshj/sftp/SFTPEngine.java Outdated Show resolved Hide resolved
@hierynomus hierynomus merged commit 4774721 into hierynomus:master Oct 23, 2023
4 checks passed
@hierynomus
Copy link
Owner

Merged, thanks for the PR!

@klelifo klelifo deleted the openssh-posix-rename branch October 23, 2023 08:58
@klelifo
Copy link
Contributor Author

klelifo commented Oct 23, 2023

You're welcome. Thanks for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants