8000 @w 18338461 Bug fix by sankarvsalesforce · Pull Request #646 · mulesoft/mule-sftp-connector · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

@w 18338461 Bug fix #646

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue 8000 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
Merged
merged 22 commits into from
Jun 10, 2025
Merged

@w 18338461 Bug fix #646

merged 22 commits into from
Jun 10, 2025

Conversation

sankarvsalesforce
Copy link

Hi, thank you for your work. We understand that you want to merge your changes and move on from this issue, but we also want to maintain the safety, readability, and testability of our code. Because of this, we kindly ask that you confirm that you have fulfilled the prerequisites for each category of activity before merging your changes.

If the Pull Request has a dependency update:

  • I have read the Release Notes for the new version (and intermediate versions if the jump would include more than one). Don't blindly trust that the dependency will honor SEMVER, always read carefully the release notes
  • Java 8+ support is maintained
  • The new version has no vulnerabilities
  • A team member reviewed the Pull Request.
  • No backward compatibility is broken.
  • If it is a "provided" dependency, it has been checked that if it is suggested, the version number is the same.

For ALL the Releases:

  • There is a task in GUS for this change.
  • The corresponding Release Notes have been written and shared with the documentation team.
  • The new code has tests and they have before and after methods to be sure that you are working with a clean environment and that you are leaving the environment clean after they finish.
  • If you are using reflection, create a test to call the methods you are invoking, this way, if there is a change in the API, the test will be able to detect it.
  • If the project has a parent pom and modules, the release pipeline only uploads the modules, so, please, make sure that the parent was deployed in the repositories before considering this release finished.
  • Notify in the Slack Channel that the release is complete, so the Docs team can merge the Documentation and Release Notes PR. The docs team always checks in Anypoint Exchange that the connector is released before merging the release notes.
  • Add the connector build to GUS, and assign that build to the GUS ticket.
  • Create a change case request, based on the Change Case Management doc https://docs.google.com/document/d/1tMJlTTZLaXMLiYwxlMBFY1yJwBmejhc4-IP8HAXgDfY/edit#

NOTE: (applies only for Core-Connectors connectors and modules) The release process ends when you merge the pull request that updates the support branch to the new snapshot, please don't forget to do this.

Patch Version:

  • The Pull Request has been peer-reviewed.
  • No backward compatibility is broken.
  • Documentation: (Required) Release Notes PR with compatibility table. Share it with the Docs team.

Minor Version:

  • Have a document with the specification of the release.
  • Create a slack channel (eg: rl-conninfra-sftp-1.1.1-new-chiper) to coordinate the release of this version with the documentation team and the architects of our team (at least one week before the release). Share the spec doc with the docs team.
  • The documentation fork has been done and has been updated with the changes related to the new functionalities.
  • The pull request has been reviewed by a peer and the team leader.
  • No backward compatibility is broken.
  • Public Documentation: Minor releases nearly always require a revision of the documentation because the release usually includes new features as well as bug fixes. Even if there are no user-facing changes, documentation has to be versioned for every minor release.
    • (Required) Release Notes PR with compatibility table. Share it with the Docs team.
    • (Likely required) Reference guide. Run the script and share the file with the Docs team.
    • (Assess) Often a minor release impacts UI and examples, so the documentation (user guides, examples, troubleshooting guides) should be reviewed and updated accordingly. Notify the docs team if any updates are needed.

Major Version:

  • Have a document with the specification of the release.
  • Create a slack channel (eg: rl-conninfra-sftp-2.0.0) to coordinate the release of this version with the documentation team and the architects of our team (at least one month before the release). Share the spec doc with the docs team.
  • The documentation fork has been made and updated with the changes related to the new functionalities and possible compatibility problems with the previous version.
  • The systems properties have been removed, detailing in the documentation the changes in the default behavior of the product.
  • The TODO comments of the code have been reviewed.
  • A colleague, the team leader, and the team architect have reviewed the Pull Request.
  • Public Documentation: Major releases break backward compatibility and should never be released without documentation, as this breaks the user experience.
    • (Required) Upgrade and Migration Guide: Every major release must have a detailed upgrade guide (template) with information about what was changed and what steps the user has to take to ensure the connector works as expected. Share the details with the Docs team so they can update the guide.
    • (Required) Release Notes PR with compatibility table. Share it with the docs team.
    • (Required) Reference guide. Run the script and share the file with the Docs team.
    • User guide updates, new screenshots or examples (if applicable), share the updates with the docs team.

@sankarvsalesforce sankarvsalesforce requested review from a team as code owners May 29, 2025 15:40
neetya26
neetya26 reviewed Jun 9, 2025
View reviewed changes
src/test/java/org/mule/extension/sftp/internal/source/SftpDirectorySourceTest.java

@Test
void testPollRetriesOnChannelBeingClosed() throws Exception {
SftpDirectorySource source = new SftpDirectorySource();
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems common in all tests - maybe we should extract it out in before test?

neetya26
neetya26 reviewed Jun 9, 2025
View reviewed changes
src/main/java/org/mule/extension/sftp/internal/connection/SftpClient.java Outdated
@@ -116,7 +115,7 @@ public class SftpClient {
private String cwd = "/";
private static final Object LOCK = new Object();
private String home;
private long heartBeatInterval = 30000;
private long heartBeatInterval = 10000; // 10 seconds heartbeat interval
Copy link
Contributor

Choose a reason for hiding this comment

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

why did we reduce this interval?

neetya26
neetya26 reviewed Jun 9, 2025
View reviewed changes
src/main/java/org/mule/extension/sftp/internal/connection/SftpClient.java
synchronized (LOCK) {
disconnect();
// Reinitialize client
if (nonNull(proxyConfig)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a similar code block on line 141 - if needed should we extract this in a separate method?

neetya26
neetya26 reviewed Jun 9, 2025
View reviewed changes
src/main/java/org/mule/extension/sftp/internal/connection/SftpClient.java Outdated
if (nonNull(proxyConfig)) {
client = ClientBuilder.builder()
.factory(MuleSftpClient::new)
.randomFactory(PRNGAlgorithm.AUTOSELECT.getRandomFactory())
Copy link
Contributor

Choose a reason for hiding this comment

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

Also in the code block on line 141, the random factory is set in a different way - are both the codes doing the same thing?

neetya26
neetya26 reviewed Jun 9, 2025
View reviewed changes
src/main/java/org/mule/extension/sftp/internal/connection/SftpClient.java
// Reconnect and authenticate
login(username);
// Retry the original operation
return new SftpFileAttributes(uri, sftp.stat(path));
Copy link
Contributor

Choose a reason for hiding this comment

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

should we instead call getFileAttributes method her which also handles exceptions?

neetya26
neetya26 reviewed Jun 9, 2025
View reviewed changes
src/main/java/org/mule/extension/sftp/internal/source/SftpDirectorySource.java
extractConnectionException(e).ifPresent(pollContext::onConnectionException);
if (isChannelBeingClosed(e)) {
try {
fileSystem = cleanUpAndReconnectFilesystem(pollContext, fileSystem);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is also repeated since line 199, should this be extracted too?

@sonarqube-as-a-service sonarqube-as-a-service
Copy link

SonarQube Quality Gate

Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

82.5% 82.5% Coverage
0.0% 0.0% Duplication

neetya26
neetya26 approved these changes Jun 9, 2025
View reviewed changes
Copy link
Contributor
@neetya26 neetya26 left a comment

Choose a reason for hiding this comment

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

LGTM

@sankarvsalesforce sankarvsalesforce merged commit b9872b6 into main Jun 10, 2025
7 checks passed
kartik-sharma-sf pushed a commit that referenced this pull request Jun 17, 2025
@kartik-sharma-sf
Merge pull request #646 from mulesoft/fix/W-18338461
c847334 
@w 18338461 Bug fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neetya26 neetya26 neetya26 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sankarvsalesforce @neetya26
0