I want to cleanup and refactor this library, discussions are OPEN!

[robinrodricks]

Intro

Here are things that I'd like to cleanup going forward. Since this has ramifications on all existing projects using FluentFTP, and since these will be major breaking changes to the API, I'd like community feedback on these changes so that they are not hasty, disruptive, or out-of-touch with the actual userbase.

I would like these to be part of the v40 release so its easy for existing users to understand that v39 is the "legacy API" and that v40+ will use the "newer API". I have never undertaken such a large systematic refactoring before.

Topic 1

• Problem: We have a lot of old/weird/unused functionality that is probably broken or not well tested or not being used.
  • EnableThreadSafeDataConnections - Crazy feature which spawns new FTP connections for each file transferred
  • QuickTransferLimit - Implemented but disabled
  • FtpClient.Connect(Uri) - Static constructors
  • FtpClient.GetPublicIP() - Static methods
  • DereferenceLink - does anyone even use this?
  • 200 different constructors for FtpClient - I would just like 1 or 2 constructors for common patterns.
• Solution: Drop these features entirely.

Topic 2

• Problem: We have a massive set of properties directly within the FtpClient class which clutters up its API.
• Solution: Store all configuration setting properties into child classes whose references are stored within the FtpClient. This groups up the properties into logical categories and also eliminates them from the FtpClient API.
  • client.Connect() becomes client.Connect(FtpProfile)
  • client.FXPProgressInterval becomes client.FXP.ProgressInterval
  • client.DownloadZeroByteFiles becomes client.Download.ZeroByteFiles
  • client.TransferChunkSize becomes client.Download.ChunkSize and client.Upload.ChunkSize

Topic 3

• Problem: Most FTP servers don't support directly downloading/uploading from an absolute path (eg: /root/animals/duck.txt, therefore we should always set the working directory to the folder (eg: /root/animals/) and then download the file (duck.txt). And we should not allow the user to manually modify the working directory because it would conflict with this system.
• Solution: Drop support for GetWorkingDirectory and SetWorkingDirectory and instead always automatically set the working directory before file transfers or even getting the file size, etc. This would even fix issues where there are spaces in directory paths, and many other things. It would put us in parity with FileZilla.

Topic 4

• Problem: Our logging is a mess. We should be able to integrate with NLog and other standard .NET loggers.
• Solution: Drop support for FtpTrace and its outdated API and instead support a client.Logger property which can be set to our default logger or an NLog instance, etc.

Topic 5

• Problem: Poor implementation of timezone conversion. Fixed implementation that does not allow users to customize it.
• Solution: Implement a strategy pattern so users can select from:
  • NoTimeConversion
  • TimeZoneConversion - a new method that allows for DST to be observed (missing feature)
  • TimeOffsetConversion - users specify which is their PC timezone and the server timezone and we do simple math to convert it (current method)

Topic 6

• Problem: We have 2 implementations of each function, a synchronous version and an async version. This causes bloat, mistakes/mismatches in the implementation, and weird behavior.
• Solution: Drop support for synchronous API. Drop support for <.NET 3.5 as it doesn't support async.

Topic 7

• Problem: Too much bloat with older IAsyncResult style async functions.
• Solution: Remove it. Anyone who wants async can just use async/await.

Lets talk!

Please start the discussions below! Add your comments and mention which topic you are talking about. You can comment on any topic(s) that you like.

[robinrodricks]

@FanDjango @tommysor @ilc-ilopez @fire-lizard @daviddenis-stx @hez2010 @n0ix @aliquid @wakabayashik @Mortens4444 @radiy @WolfspiritM @Lukazoid @iamjay @peterfortuin @taoyouh @artiomchi @sierrodc @jblacker @L3Z4 @jnyrup @Adhara3 @brucemcmillan

[Adhara3]

Hi,

Foreword

as I already suggested in other issues, I would try, whenever possible, to avoid behaviour by enums, favoring a strategy pattern instead. This would have the benefit to help (not solve) Topic 2 for example, moving specific configuration only where necessary. To be clear I would favour something like new XxxFtpClient() which is an FTPClient of type Xxx ad internally it is built by injecting specific strategies for that server type. It's not easy but I see a potentially big architectural advantage.

Topic 3

This is the one I am mostly involved in. I agree that not allowing browsing the tree would help and that would also be strategy oriented. User provides the full path, it's up to the implementation use it as it is or navigate to the folder and then download

Topic 4

Having a simple IFtpClientLogger interface is a good way to go, you just need 5 or 6 logging methods and the user can easily plug in any implementation. Do not add any specific dependency.

Topic 6

I am not a huge fan of async code, to put it mild. But I get this is a feature largely used nowadays, the only thing I could say is that at the moment I'm only using sync methods from FluentFTP.

Hope this helps A

[robinrodricks]

Topic 3/4/5: Agreed (more or less)

Forward;

as I already suggested in other issues, I would try, whenever possible, to avoid behaviour by enums, favoring a strategy pattern instead.

It sounds pretty cool and could fix issues by offering simple and complex strategies. Assuming we have just one FtpClient for simplicity and all features are implemented in modular strategies, suggest the implementation. For example how would functionality be divided into strategies. For something like downloading a file, we don't really have strategies. Unless we should? Like SimpleFileDownload and ChunkedFileDownload etc. And what should the API look from the client? client.Downloader = new SimpleFileDownload()? Please suggest the concrete implementation so I can get a better idea.

[Adhara3]

I'm not expert on FluentFTP client internals, so I'm still talking in a very theoretical way, based mainly on the Find What Is Varying and Encapsulate It basic principle. So I would ideally try to have something like the following (pseudocode):

public class FTPClient
{
  public FTPClient(IDownloadStrategy downloadStrategy, IUploadStrategy uploadStrategy, IAuthenticationStrategy authStrategy)
  {
    // Of course, as many strategies as required
  }

  public void Download()
  {
    // You can du stuff here before calling the strategy, but it MUST be server independent, guaranteed
    m_downloadStrategy.Download();

    // Or the call could contain another strategy
   m_downloadStrategy.Download(m_authStrategy);

    /* This is a key point, strategies could have other strategies from constructor or at runtime, 
it really depends on the scenario. Passing at runtime may allow different behaviour, there is no 
good or bad, passing in construction gives the dev a message: this is readonly and cannot be 
changed, so depending on the scenario may be the right choice */
  }
}

public static class FritzBoxFtpClient
{
  // Here custom properties for this kind of client, should help with cluttered config
  // e.g. TransferChunkSize goes into the proper stategy
  public static FTPClient Create() 
  {
    return new FTPClient(); // Inject strategies for this specific client
  }
}

I do expect having, for each strategy type, a number N of implementations which is less than the actual number of clients, i.e. I guess a BasicDownloadStrategy may be reused for multiple clients. I used the static factory method to avoid inheritance and because that would force to put all different behaviours into strategies and not in method overloading (mixing the two would kill code readability).

The other advantage is that this pattern would allow native extendibility. E.g. a user has a new client, maybe a custom one. He can create his own strategies and inject into FTPClient constructor and he should have something working. Basically, you would end up having some core classes + a bunch of add-ons (each client).

Moreover, async is in the syntax, so it doesn't work, but a Download strategy could be one file at a time and another implementation may be EnableThreadSafeDataConnections, basically instead of an option, it's a behaviour. Topic 5 would also fall into this. You provide the API (i.e. the interface) and, if you like, a couple of very basic implementations, but then a user can always customize if required by injecting a different implementation.

The hard part is:

• exact opposite of what you have today, instead of a huge single FTPClient class that does everything, several smaller classes. This helps with Unit testing, though
• modularity for working properly needs to be as atomic as the smallest change, so it may end up being a bit iterative, e.g. IDownloadStrategy may not be enoght, you may need to split it (or to provide parts to it) like IPathBuildingStrategy or IServerResponseStrategy, just to give an idea

[FanDjango]

Just coming from my insular view for the IBM parts of this:

I am worried about:

Topic 3

Anyone writing a FTP Client GUI and wanting to give the user a way to walk the server-OS's file system will want to have these two functions, right? And the IBM parts really heavily use PWD/CWD.

Topic 6

I am not a huge fan of async code, to put it mild. But I get this is a feature largely used nowadays, the only thing I could say is that at the moment I'm only using sync methods from FluentFTP

Same for me.

I find it strange that C# religion goes for "not duplicating code" but makes it so difficult to have the same basic algorithm present in both sync and async modes. Certainly in an C(++) environment I could envision a single code set (using the preprocessor) that represents both flavors in a single source. How to do in C#?

[robinrodricks]

Topic 3

Anyone writing a FTP Client GUI and wanting to give the user a way to walk the server-OS's file system will want to have these two functions, right? And the IBM parts really heavily use PWD/CWD.

Maybe we can have a strategy for this?

• ManualDirectoryStrategy - the current get/set PWD/CWD
• AutomaticDirectoryStrategy - a new one which can auto set the dir based on the file commands

Names are just suggestions, you can suggest better ones. I also don't like having the word "Strategy" as a suffix, is there a better naming scheme? <Variant> <Subject> "Strategy" seems like a very Java-esque too-many-words naming scheme and I would like a better one!

For backwards compatibility, we could support all the older constructors, which would be implemented like:

public FtpClient() {
	this.DownloadStrategy = new ChunkedDownloadStrategy();
	this.UploadStrategy = new ChunkedUploadStrategy();
	this.PathStrategy = new ManualDirectoryStrategy();
	....
}

Topic 6

I am not a huge fan of async code,

Same for me, however lots of contributors were excited to contribute async versions for some reason... And it makes the library look more "modern". I don't know why users can't just create a background worker/thread and use the sync API, maybe its because of the whole node.js "everything is an async/non-blocking operation" pattern that took the world by storm.

[Adhara3]

This may be silly or very naif. What about adding sync stuff as ExtensionMethods? That would allow keeping the class basically sync, users would be happy as they can call the method directly on the object.

A

[robinrodricks]

This may be silly or very naif. What about adding sync stuff as ExtensionMethods? That would allow keeping the class basically sync, users would be happy as they can call the method directly on the object.

No, that would not solve the main problem of duplicating code.

[hez2010]

I would like to see

1. Drop net3.5 support and keep only async variant of APIs, for those who want to use blocking sync call, they can always do FooAsync().ConfigureAwait(false).GetAwaiter().GetResult().
2. Bringing IAsyncEnumerable support again to support "list streaming", without the dependency of Rx/Lx.Net.

[robinrodricks]

Bringing IAsyncEnumerable support again to support "list streaming", without the dependency of Rx/Lx.Net.

I'm not against it but it caused #703 and so it was reverted. If you have a solution for it without causing that issue, you can please do it and file a PR.

[Adhara3]

No, that would not solve the main problem of duplicating code.

Ok but then this is why

I don't know why users can't just create a background worker/thread and use the sync API,

If they could, we also could in an extension method, right?

[robinrodricks]

I don't like providing main library API as extension methods. Extensions are meant when the third parties want to extend something without inheriting it.

[sierrodc]

Bringing IAsyncEnumerable support again to support "list streaming", without the dependency of Rx/Lx.Net.

I'm not against it but it caused #703 and so it was reverted. If you have a solution for it without causing that issue, you can please do it and file a PR.

Hi, According to #703, the issue was 'only' the reference to System.Linq.Async.

Please take a look here: https://github.com/RobertoSarati/FluentFTP_asyncenum. (fork reset to release 33.1.6). As far as I can see the package System.Linq.Async is referenced only if ASYNCPLUS is defined, that is netstandard 2.0/2.1 and net5.0. I've removed the package reference and without any other change I was able to compile the project. I've created a .net6 console app referencing the project and I'm able to use GetListingAsyncEnumerable without any issues. Maybe I'm missing the root cause (ps: I've removed net4, net45, net50 from csproj because I don't have them installed on my machine).

Btw, do you still want to support net3.5, net4, net45? According to microsoft .net framework < 4.6.2 are no more supported. So, what if target framework list is just [netstandard2.0;netstandard2.1;net50+]?

[robinrodricks]

I've removed the package reference and without any other change I was able to compile the project. I

Ok that sounds cool @sierrodc , can you check if GetListingAsyncEnumerable works in https://www.nuget.org/packages/FluentFTP/39.2.0 ?

Btw, do you still want to support net3.5, net4, net45? According to microsoft .net framework < 4.6.2 are no more supported. So, what if target framework list is just [netstandard2.0;netstandard2.1;net50+]?

That is what I am asking the community. For myself I don't mind as I only use the latest .NET framework.

Unrelated: Can I at least delete the older IAsyncResult style async functions from the library to remove bloat? Even I have never used them personally. Plus anyone who wants async can just use async/await.

[sierrodc]

Can I at least delete the older IAsyncResult style async functions from the library to remove bloat? Even I have never used them personally. Plus anyone who wants async can just use async/await.

Imho the "sync/async" programming style should be as close as possible to what is today available in HttpClient so, for sure, no IAsyncResult. About sync method... I'm fine to have only aysnc/await (I'll use only these ones and I dont' want many methods in the intellisense dropdown), but if the community wants to keep sync style, I've no objections. But for me, we can switch to implement only async/await methods. Later I'll check if GetListingAsyncEnumerable works in current master branch.

[robinrodricks]

What I've planned for v40:

To delete

1. EnableThreadSafeDataConnections
2. QuickTransferLimit
3. FtpClient.Connect(Uri)
4. FtpClient.GetPublicIP()
5. DereferenceLink
6. DereferenceLinkAsync
7. IAsyncResult functions (async implementation for .NET 2 and older)
8. FtpTrace

To refactor:

• client.Settings (class to hold all settings)

• client.Logger (strategy pattern)

• client.TimeConversion (strategy pattern)

  • NoTimeConversion
  • TimeZoneConversion
  • TimeOffsetConversion

• Split FtpClient into 2 clients - FtpClient and AsyncFtpClient

  • Solves the issue of this messy API:

To add:

• client.Settings.ToJson - using System.Text.Json
• client.Settings.FromJson
• client.Settings.ToXml - using XmlSerializer
• client.Settings.FromXml

[robinrodricks]

Do you guys think this is a decent solution?

At present I don't see any advantage to the user for splitting the download/upload/FXP parts, because even if I did refactor the "downloader" to use strategy pattern, I don't have any alternate strategies to offer the user.

It would just force the user to change from: client.DownloadFile to client.Downloader.DownloadFile so that does not offer any major advantage at present.

[sierrodc]

1;3;4;7;8: ok 2;5;6: never used => ok

• About logging: Such as HttpClient, why not just implement an EventSource? I think all libraries implement proper sinks (nlog, serilog ...). Really simple explanation here (it is the same implementation used by HttpClient and other .net libraries).
• About TimeZone conversion: In my opinion the library should not handle conversions at all. There are other packages focused on this functionality. BUT if there are ftp servers that specify the timezone I think your solution is fine.
• About AsyncClient and SyncClient: Fine for me.

[jnyrup]

First of all thanks for raising this discussion.

Topic 4: Logging

Building on top of the suggestion by @Adhara3 here's a rough prototype of how the logging could be structured

Define an interface in FluentFtp to separate the functionalities of FtpTrace.

interface IFluentFtpLogger
{
    void Log(FtpTraceLevel eventType, object message);
}

class NoOpLogger : IFluentFtpLogger
{
    public void Log(FtpTraceLevel eventType, object message)
    {
        // The default logger that does nothing.
    }
}

class ConsoleLogger : IFluentFtpLogger
{
    public void Log(FtpTraceLevel eventType, object message)
    {
        // Write to Console
        throw new NotImplementedException();
    }
}

class FileLogger : IFluentFtpLogger
{
    public void Log(FtpTraceLevel eventType, object message)
    {
        // Write to file
        throw new NotImplementedException();
    }
}

/// <summary>
/// Allows combining multiple loggers
/// </summary>
class CompositeLogger : IFluentFtpLogger
{
    private readonly IFluentFtpLogger[] m_loggers;

    public CompositeLogger(params IFluentFtpLogger[] loggers)
    {
        m_loggers = loggers;
    }

    public void Log(FtpTraceLevel eventType, object message)
    {
        foreach (var logger in m_loggers)
        {
            logger.Log(eventType, message);
        }
    }
}

To allow hooking into the widely used ILogger<T> interface from Microsoft.Extensions.Logging.Abstractions we publish a separate package FluentFtp.Logging that simply contains an adapter to bridge between ILogger<T> and IFluentFtpLogger. This avoids forcing a dependency on Microsoft.Extensions.Logging.Abstractions, but still makes it easy to integrate with popular logging libraries like Serilog, NLog, etc.

class LoggerAdapter : IFluentFtpLogger
{
    private readonly ILogger<IFtpClient> logger;

    public void Log(FtpTraceLevel eventType, object message)
    {
        var loglevel = ConvertLogLevel(eventType);
        if (logger.IsEnabled(loglevel))
        {
            logger.Log(loglevel, message.ToString());
        }
    }

    private static LogLevel ConvertLogLevel(FtpTraceLevel eventType)
    {
        return eventType switch
        {
            FtpTraceLevel.Verbose => LogLevel.Trace,
            FtpTraceLevel.Info => LogLevel.Information,
            FtpTraceLevel.Warn => LogLevel.Warning,
            FtpTraceLevel.Error => LogLevel.Error,
            _ => LogLevel.Information // Fallback
        };
    }
}

Topic 6: Target frameworks

FluentFtp currently supports frameworks that have been EOL for years. Retaining that support seems to add significant complexity (currently 492 #ifs).

• net20, net40 and net45 are no longer supported.
• net35 SP1 is technically still supported until 2029, but I would recommend for FluentFtp to drop support for it.
• .net 5 is no longer supported.

I we want to support older but still supported frameworks, we could go with net50;netstandard2.1;netstandard2.0;net472;net462.

https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/cross-platform-targeting https://github.com/dotnet/docs/issues/8896 To see what TFM of FluentFtp an application will use, use Nuget Tools

Topic 6: Async

async is important in modern C# to avoid thread starvation, so I highly recommend not to drop that part. It cannot always be delegated (nicely) to a background thread, e.g. a web api that as part of its work downloads from an FTP server.

In regards to whether we should also keep the sync APIs, other clients have mostly dropped sync versions.

• HttpClient is mostly async - some sync APIs were added in .NET 6, but mainly for backwards compatibility or legacy code and because httpClient.Send() could be implemented better than httpClient.SendAsync().Wait().
• AWS S3 client has transitioned to async only.

Topic 7: Removing IAsyncResult

🔥🔥🔥

Concurrency

One thing that surprised me when using FtpClient compared to e.g. HttpClient was that is inherently not suited for concurrent actions as it carries state, e.g. LastReply.

I haven't thought this through, but maybe something like this would di

var client = new FtpClient();

... // setup the ftp client 

using FtpConnection connection = client.CreateConnection();
byte[] content = await connect.DownloadAsync();

I'm aware that this would be yet another major restructuring of the project, so this is mostly a shower thought for now.

[robinrodricks]

@jnyrup Thanks for the long and detailed answer!

Topic 4: Logging

I dislike "too many nuget packages" so would it cause any harm to add a direct dependency on Microsoft.Extensions.Logging.Abstractions ?

And then do as you proposed above. (NoOpLogger, FileLogger...) Maybe one change would be instead of having a single instance of logger, we could have a list of loggers directly in the client for simplicity? Making CompositeLogger not required.

API would be: client.Loggers = new List<ILogger{....} or client.AddLogger(myLogger)

Topic 6: Target frameworks

I agree (mostly):

Current frameworks: net60;net50;net45;net40;net35;net20;netstandard1.4;netstandard1.6;netstandard2.0;netstandard2.1

Proposed frameworks: net60;net50;net45;net40;net35;netstandard2.0;netstandard2.1

Topic 6: Async

I am planning on splitting FtpClient into 2 clients - FtpClient and AsyncFtpClient, do you agree with this? Both will inherit from BaseFtpClient which will have some reused properties/functions.

Concurrency

Yes concurrency is not currently supported. If you try to do multiple simultaneous operations on a single FTP client connection it will break (as per limitations of FTP protocol).

[Adhara3]

I would probably drop the FtpTraceLevel way to go, I was thinking more to something like this

public interface IFtpLogger
{
  void Trace(string message); // I personally see no advantages in passing an object
  void Debug(string message);
  void Info(string message);
  void Warning(string message);
  void Error(string message);
  void Error(string message, Exception exception);
}

I would also probably provide only the composite and the NoOpLogger. I also agree that if a dependency should be added, it should be added in a separate, dedicated package.

@robinrodricks

At present I don't see any advantage to the user for splitting the download/upload/FXP parts, because even if I did refactor the "downloader" to use strategy pattern, I don't have any alternate strategies to offer the user. It would just force the user to change from: client.DownloadFile to client.Downloader.DownloadFile so that does not offer any major advantage at present.

I would not insist any further on this, I just wanted to clarify that:

1. Strategy may not be the right term, but the idea is to hide different behaviours behind small classes to improve testability, favour aggregation and modularity and have a cleaner code. The idea is that there should be a "strategy" every time in current code you do if(serverType == xxx)
2. It is an implementation detail, so users are not involved directly, FTPClient would act as a proxy for the strategies. Again, this would just be a more object oriented way of building the software.

Concurrency

This is interesting. As a user, the async interface would give me the natural feeling that the client is concurrent, so be careful here. As suggested by @jnyrup is a user expect to run the client as the HttpClient object, then it may be worth thinking a way to overcome this, maybe providing a FtpClientPool? Just as food for thoughts

A

[jnyrup]

Topic 4: Logging

I dislike "too many nuget packages" so would it cause any harm to add a direct dependency on Microsoft.Extensions.Logging.Abstractions ?

In terms of functionality of FluentFtp taking a direct dependency on M.E.L.A should "just work", but in general there are more considerations to take into account when adding extra dependencies.

When I'm about to add an additional nuget package, I have to evalute all transitive package references, in order to get a full view on licenses, vulnerabilities, maintenance status, etc. to avoid creating technical debt/security issues from day one. For M.E.L.A this should not be a problem.

Increased size of your application as extra unused dlls are included. For M.E.L.A it should be about 62K if FluentFtp is the only nuget referencing it.

Note, the earliest non-deprecated of this nuget package targets netstandard20 which is net461 or later. So taking a direct dependency on Microsoft.Extensions.Logging.Abstractions prevents FluentFtp from targeting net35/net40/net45 (a non-issue to me since I would drop those targets).

So I think it mostly comes down to ones view on providing a single package with all batteries included or a more modular approach, where consumers don't have to depend on something they don't use.

And then do as you proposed above. (NoOpLogger, FileLogger...) Maybe one change would be instead of having a single instance of logger, we could have a list of loggers directly in the client for simplicity? Making CompositeLogger not required.

API would be: client.Loggers = new List<ILogger{....} or client.AddLogger(myLogger)

Established logging frameworks already provide knobs to combine multiple loggers (sinks) into a single ILogger, so the CompositeLogger should only be necessary when using basic included loggers FileLogger and ConsoleLogger. We could even go in the more extremen direction and say implementations of how to log is not a responsibility of FluentFtp and not provide any logger, but only an interface and the NoOpLogger().

As an endorsement, the general "best-practice" is to use constructor-based dependency injection, but since a NoOpLogger can serve as a reasonable default property-based injection seems justified for FluentFtp.

So I would probably go with public XyzLoggingInterface Logger { get; set;} = new NoOpLogger();.

Two benefits of using the methods exposed on ILogger are the support for:

• structured logging.
• IsEnabled, which may make it cheaper when logging is disabled (e.g. having different loglevels between dev and prod).

Topic 6: Target frameworks

Proposed frameworks: net60;net50;net45;net40;net35;netstandard2.0;netstandard2.1

I shouldn't be necessary to have both net50 and net60, unless you want to use net60 specific features or import different nuget packages based on the framework, as a net60 applications can consume a net50 nuget. Adding unnecessarily targets to a nuget package just increases the binary size of the nuget package.

Out of curiosity, why do you intend to keep support for net35, net40 and net45?

Topic 6: Async

I am planning on splitting FtpClient into 2 clients - FtpClient and AsyncFtpClient, do you agree with this? Both will inherit from BaseFtpClient which will have some reused properties/functions.

I wouldn't do that. It e.g. prevents reusing the same client for sync and async invocation paths. I'm curious what the intended goal for this is. If the intention is to reduce the number of available methods listed with intelliSense, you could consider creating two interfaces.

public interface IAsyncFtpClient
{
    public Task<byte> DownloadAsync();
}

public interface ISyncFtpClient
{
    public byte Download();
}

public class FtpClient : IAsyncFtpClient, ISyncFtpClient
{}

Strategies

The idea is that there should be a "strategy" every time in current code you do if(serverType == xxx)

I agree here that ideally, the FtpClient should not be aware of which server it is connected to, but just that it is entering server dependent behavior and hence delegate the implementation to a dedicated handler.

[robinrodricks]

Topic 4: Logging

Agreed on MELA.

I prefer "batteries included" as far as possible so I would like to implement all these in FluentFTP itself:

• NullLogger
• FileLogger
• ConsoleLogger
• CompositeLogger

Topic 6: Target frameworks

Out of curiosity, why do you intend to keep support for net35, net40 and net45?

Well just to support a broader user base. I was aware of enterprises that find it harder to upgrade to newer .NET versions. What is your take on this? If we are to support MELA then we would be forced to drop net 3.5/4/4.5 which is ok with me provided its ok with the user base.

I specifically wanted net5/6 so that users with those 2 frameworks can use FluentFTP without forcing you to upgrade to THE latest version. For example some businesses don't want to keep re-purchasing the latest VS version.

Topic 6: Async

I wouldn't do that. It e.g. prevents reusing the same client for sync and async invocation paths.

Would anyone do that? A lot of the users on this thread mentioned they use either sync or async but not both. I can't imagine how you would use both, considering that FluentFTP does not allow concurrent operations anyway. In fact I specifically don't like this method of mixing sync and async code as it may not be well tested usage.

I'm curious what the intended goal for this is.

1. To clean up and reduce the API surface of each client class. I prefer less interfaces so that's not something I'd like to do.
2. I would also like to remove the Async suffix for all class methods within the AsyncFtpClient class.
3. Also to help cleanup the codebase as I will be splitting the files into separate code files for sync/async versions.

Is it ok to drop the 'Async' suffix for async methods?

From 1:

Of course, there are always exceptions to a guideline. The most notable one in the case of naming would be cases where an entire type’s raison d’etre is to provide async-focused functionality, in which case having Async on every method would be overkill, e.g. the methods on Task itself that produce other Tasks.

From 2:

If there is both non-async and async method that return the same thing I suffix the async one with Async. Otherwise not.

Strategies

I agree here that ideally, the FtpClient should not be aware of which server it is connected to, but just that it is entering server dependent behavior and hence delegate the implementation to a dedicated handler.

We already have implemented a server-specific strategy system which implements this as far as possible. Some zOS specific code is still remaining in the core which should be refactored out as far as possible.

[jnyrup]

Topic 6: Target frameworks

I was aware of enterprises that find it harder to upgrade to newer .NET versions. What is your take on this?

MS ended support for .net2.0 over 10 years ago and for net40 over 5 years ago. net462 was introduced 5 years ago, so I would no longer regard it as a "newer .NET versions". I admire your concern about not only supporting the latest and greatest (this a quite debated topic among OSS maintainers), but at one point most OSS projects drop some support in order to move forward and keep the project fun and maintainable. So there no "right" thing here, e.g. Newtonsoft.Json still supports .net20. Personally I would say that those "enterprises" can either use the current version of FluentFTP or have to upgrade their application to at least net462 to get the latest and greatest of FluentFTP.

I specifically wanted net5/6 so that users with those 2 frameworks can use FluentFTP without forcing you to upgrade to THE latest version. For example some businesses don't want to keep re-purchasing the latest VS version.

.NET5 is targeted in FluentFTP to add support for TLS 1.3. But net6 applications can also consume that. Adding a net6 target only increases the binary size of the FluentFTP nuget package as it includes a fluentFtp.dll for both net5 and net6. Similarly, you don't have to add a net7 target when it is released in November, they can also consume a .net5 package.

Topic 6: Async

I think I understand your rationale now, to help users not mixing sync and async calls. It doesn't sound unreasonable to me, but I can't recall having seen that design choice elsewhere.

[robinrodricks]

Topic 6: Async

very nit picking, but splitting the code into two classes does not make it more tested.

As in, in our tests we only test async methods and sync methods separately. Combining them in the same FtpClient might produce weird/undefined behavior.

It doesn't sound unreasonable to me, but I can't recall having seen that design choice elsewhere.

Its more of a thing to clean up the API and codebase, rather than for testing actually.

And to go in the direction where we have 2 clients. Maybe sometime say 5 years from now, "no one" uses FtpClient then we can outright delete that codebase and have just the dedicated AsyncFtpClient. I'm also tired of seeing long method names like client.DownloadDirectoryAsync when you could just have client.DownloadDirectory. Also it forces devs to make the choice of sync/async up at the start and to stick to it.

[robinrodricks]

Topic 6: Target frameworks

Thanks for your long justification and explanation. As you said, its such a debatable thing. I guess because of MELA and other reasons, I'm ok dropping support for older .NET frameworks. I hope no companies cry because we no longer support their [outdated] .NET version.

So are you proposing this?

net60;net50;netstandard2.0;netstandard2.1

[jnyrup]

From how I read the code you could probably target as few frameworks as net50;netstandard2.0;netstandard2.1.

This code snippet currently distinguishes between .net frameworks and others, so if we need to keep that distinction we also need to have net462 or net472. https://github.com/robinrodricks/FluentFTP/blob/51e1d85b220ce8d16c10b947778ebd32b4c63a26/FluentFTP/Client/FtpClient_Stream.cs#L1174-L1186

[FanDjango]

Hmmpf. One of my customers (Fortune 500) is still on 4.7.2

[robinrodricks]

Can we support the same frameworks MELA supports?

net60;netstandard2.0;net461

Alternatively is there a way we can not use MELA for logging interface, then we will not be forced to use these platforms and could support net50 too.

[jnyrup]

Hmmpf. One of my customers (Fortune 500) is still on 4.7.2

net472 can consume netstandard2.0 nuget packages. https://nugettools.azurewebsites.net/6.0.0/get-nearest-framework?project=net472&package=net50%0D%0Anetstandard2.0%0D%0Anetstandard2.1

https://docs.microsoft.com/en-us/dotnet/standard/library-guidance/cross-platform-targeting#multi-targeting

For increased compatibility with supported frameworks we could go net50;netstandard2.0;netstandard2.1;net472;net462. https://github.com/dotnet/docs/issues/8896

Can we support the same frameworks MELA supports?

All targets we pick should be able to consume MELA 2.1.0.

[robinrodricks]

I'm good with this : net50;netstandard2.0;netstandard2.1;net472;net462

I'm good with MELA 2.1.0, it is non-restrictive on framework choice.

Are we all in agreement with this choice?

[FanDjango]

Hmmpf. One of my customers (Fortune 500) is still on 4.7.2

net472 can consume netstandard2.0 nuget packages.

Yes, thanks. But I wanted to say: netstandard2.0 is kind of bottom line still

I wish to use more up to date stuff but I cannot.

[robinrodricks]

Are we all in agreement with this choice? @FanDjango ?

[FanDjango]

Agreement from me?

I have no right to an opinion on this for total lack of knowledge/experience on/of this topic.

I have just checked: One of my clients actually needs application binaries that utilize .NET Framework 4.7 (not 4.7.2 as earlier stated) on systems in protected environments where updates cannot so easily be done (and won't be done). I am currently linking in the FluentFTP binaries from net45 into my application, which targets net47.

Current functionality suffices for me, I can use a frozen version status today, that is always a choice.

If you have advice for me on how to proceed beyond that, I will happily follow that.

[robinrodricks]

@FanDjango would this help? net45;net50;netstandard2.0;netstandard2.1;net472;net462

[jnyrup]

A net47 application will consume the net462 target. https://nugettools.azurewebsites.net/6.0.0/get-nearest-framework?project=net47&package=net50%0D%0Anetstandard2.0%0D%0Anetstandard2.1%0D%0Anet472%0D%0Anet462%0D%0Anet45

[FanDjango]

Thnks @jnyrup. So it seems that @robinrodricks previous suggestion net50;netstandard2.0;netstandard2.1;net472;net462 will be ok. Or in other words, net462 is my current minimum requirement.

[Adhara3]

We already have implemented a server-specific strategy system which implements this as far as possible. Some zOS specific code is still remaining in the core which should be refactored out as far as possible.

That was a very welcome first step. Beware that the base class still exposes the server type enum, this could allow having server depending code all around the code. The first question here is: what breaks if you remove the ServerType property?

[robinrodricks]

That enum is the only way for users to detect the FTP server as the handlers are internal and not accessible to library users.

[FanDjango]

Some zOS specific code is still remaining in the core

Yes, I am sorry for causing this. I believe though that at the time it was the expedient way to get the needed functionality "in there" and confirming that the approach simply works for z/OS (which can be a PITA for FTP). Now would be the time for refactoring it "out of there", I certainly agree.

@robinrodricks You are definitely the designated best for doing that, of course, but you can be assured I will be available for testing the changes on a live server.

[FanDjango]

That enum is the only way for users to detect the FTP server

I for one need this enum

[Adhara3]

I for one need this enum

I'm curious: why? I mean, on one side the lib tries to hide the server differences, on the other side it must expose the kind. I am just curious.

[FanDjango]

I understand. Hmmm. I am giving my user a GUI to browse/search the filesystem in order to find the files he wants to process. If it is a z/OS server, the semantics for file names differ in some cases (partitioned data sets), the server capabilities are (greatly) expanded, and additional functions exist (like adressing the Job Entry Subsystem). All of these I want to be "greyed out" or not presented to the user when he is NOT connected to a z/OS FTP Server.

If connected to an OS/400 type of FTP Server, there also subtle things to do differently which you might not do on FTP Servers sitting on a standard unix file system.

I dunno if there are other examples out there, but for me it looks like a "special" IBM thing (such as it always is on proprietary systems). Although: one other such system used to be TANDEM, always special things to do on that server.

So, I don't really believe in "hiding" the server difference to the degree that the library user doesn't even need to know the server type.

If this is not possible, I will just issue a SYST command to find out for myself, but since the connect logic already does that internally, why not make the results available (in some form, does not need to be an enum).

[Adhara3]

@FanDjango it's fine, I just needed some context. Thanks

[robinrodricks]

I'm curious: why? I mean, on one side the lib tries to hide the server differences, on the other side it must expose the kind. I am just curious.

The idea was to give the user maximum power. If we have some info, we allow them to read that info and do whatever they want with it. Perhaps they are building a generic FTP client and can display the "currently connected server type" to the user. Or for logging, or for error handling. So we don't try to hide anything from users. However, that being said we DO try to hide our own internal classes which they should not need to access.

[Adhara3]

It makes perfect sense. The important thing, to me, is that the enum is not used internally for stuff like if(serverType == xxx). These are my 2 cents. Cheers A

[FanDjango]

the enum is not used internally for stuff like if(serverType == xxx)

The only ones I see currently are: IBMzOSFTP and OpenVMS.

I am working on the IBM ones currently, moving them all into ServerHandler. constructs in one form or the other.

In some cases, it is not so totally clear cut how precise you want to be with the knife - the current if... constructs more or less reflect the easy way to get things working but not the best organisation of the code if you want to refactor the servertype out to an alternative function/bool or whatever.

If the highest priority is to simply get these if (serverType == xxxx) out. then fine, as a first step that can be done. But somehow I feel that perhaps one would like to replace complete functions instead of small code snippets.

Here is an example: API Function FileExists. Four separate serverHandlers inside the function or simply a big custom FileExists for z/OS, which then duplicates a lot of the original code - with the nightmare of maintaining changes to those duplications. Why so much duplication in a custom function for z/OS?: Because, if you are in the unix realm, you could just switch back to the default function.

Thus, I am thinking that perhaps in addition to serverHandler.IsCustomFunction(...) and serverHandler.CustomFunction(....), we would need a third construct that checks if a fallback to the original Function is needed.

I suggest therefore that in addition to being able to replace entire API functions or to influence small parts of them internally with booleans, we need to conditionally replace entire API functions.

Working on it... so that the cleanup is not hindered by z/OS.

[FanDjango]

the enum is not used internally for stuff like if(serverType == xxx)

I have eliminated them ALL (PR #928). Now go ahead with the original plan/discussion

[FanDjango]

Maybe we can have a strategy for this?

ManualDirectoryStrategy - the current get/set PWD/CWD AutomaticDirectoryStrategy - a new one which can auto set the dir based on the file commands

Maybe a "Freedom" strategy, for those who want to navigate, to use the listing APIs etc.

And a "AI-Guided" strategy, for those who just want to download/upload stuff ( with a well-defined local/remote path).

[robinrodricks]

Hi @FanDjango, @jnyrup, @Adhara3 and anyone else who has some time. Please check out this branch and test it and see if it works for you!

https://github.com/robinrodricks/FluentFTP/tree/v40

Please feel free to raise PRs for the v40 proposal against the new branch!!

What is done:

40.0.0

• New: Split main FTP client classes into FtpClient and AsyncFtpClient
• New: Split main FTP client interfaces into IFtpClient and IAsyncFtpClient
• New: Split common FTP functionality into BaseFtpClient
• New: Split FTP proxy classes into FtpClient*Proxy and AsyncFtpClient*Proxy
• New: FTP proxy classes moved into FluentFTP.Proxy.SyncProxy and FluentFTP.Proxy.AsyncProxy NS
• New: FTP proxy classes with fully async implementations
• New: Add Nuget dependency Microsoft.Extensions.Logging.Abstractions v2.1.0
• Modernize: Completely redesign the FTP client code organization and structure
• Modernize: Drop support for .NET Standard 1.2, 1.4 and .NET 2.0, 3.5, 4.0 and 4.5
• Modernize: Remove conditional compilation statements for unsupported platforms
• Modernize: Remove uncommon static methods FtpClient.Connect and FtpClient.GetPublicIP
• Modernize: Remove uncommon method DereferenceLink and DereferenceLinkAsync
• Modernize: Remove uncommon properties QuickTransferLimit, MaximumDereferenceCount, EnableThreadSafeDataConnections
• Modernize: Remove uncommon feature FtpListOption.DerefLinks
• Modernize: Remove obsolete hashing commands GetHashAlgorithm, SetHashAlgorithm, GetHash, etc
• Modernize: Remove obsolete async pattern using IAsyncResult
• Modernize: Move all IBM zOS logic into the IBMzOSFtpServer server handler (thanks FanDjango)
• Modernize: Move all OpenVMS logic into the OpenVmsServer server handler (thanks FanDjango)

Notes

1. I have not yet renamed the async methods to remove the Async suffix, try it out. If you feel I can go ahead then I will be renaming all the methods in the AsyncFtpClient class to remove that suffix.
2. I have not changed any of the properties
3. I have not implemented the new logging/timezone patterns

[FanDjango]

@robinrodricks 40.0.0 Ok - if I remove c# and VB examples from the solution remove the two test projects from the solution remove ssl validation and TLS connection from my z/OS Test project then I can build.

Then the entire z/OS Test Suite that I have accumulated over the last year runs fine.

Congratulations from me.

[robinrodricks]

Thanks @FanDjango, do you get serious errors with c# and VB examples, test projects? Or do you mean your own projects? What kind of errors do you get, can you list them here please.

[FanDjango]

Here's what I did, to be totally fresh and clean, no changes, no own projects at first, nothing, nada:

1. Deleted my complete fork
2. Forked anew
3. Cloned my (forked) repo into VS2022
4. Successfully DEBUG CLEAN, then REBUILD branch master, all ok.
5. switched to branch V40, after fetching from upstream into my fork
6. DEBUG CLEAN branch v40, all ok
7. REBUILD branch v40: Here are the errors:

'FtpClient' does not contain a definition for 'ConnectAsync' and no accessible extension method 'ConnectAsync' accepting a first argument of type 'FtpClient' could be found (are you missing a using directive or an assembly reference?)

Etc.

[robinrodricks]

@FanDjango You're right, all the tests and examples were broken. I fixed it now.

Do you support the idea of removing the Async suffix for methods in the AsyncFtpClient?

[FanDjango]

If there is with a class both non-async and async method that return the same thing suffix the async one with Async. Otherwise not. But if these methods are bound to two different classes, I would not suffix with Async, as this is a homomorphism.

[robinrodricks]

Done

• Asynchronous API

  • New: Drop Async suffix for all async FTP methods in AsyncFtpClient

• Logging API

  • New: Remove client.OnLogEvent and FtpTrace system
  • New: Add logger system client.Logger using industry-standard ILogger interface
  • New: Add logging settings: LogToConsole, LogFunctions, LogIP, LogUserName, LogPassword
  • New: Add Nuget dependency Microsoft.Extensions.Logging.Abstractions v2.1.0

• Config API

  • New: Remove all config settings from FtpClient and move it into client.Config object
  • New: Dedicated class to hold config settings FtpConfig to cleanup client API

[robinrodricks]

@FanDjango @jnyrup Can you review this logger implementation please?

https://github.com/robinrodricks/FluentFTP/blob/master/FluentFTP/Client/BaseClient/Log.cs#L15

Usage: To set a logger you can use client.Logger = XYZ and/or client.LogToConsole = true

[Adhara3]

Usage: To set a logger you can use client.Logger = XYZ and/or client.LogToConsole = true

Console is a possible output, so the former includes the second. I agree with keeping options such as LogFunctions because these indicates to the internals what should be logged but where to log should be kept in a single option (Logger) IMHO. If the user wants to log to console he should provide a ConsoleLogger. To be (very) strict, even this

#if DEBUG
	Debug.WriteLine(message);
#endif

may not be there but shoud be turned into a logger.

Moreover, be careful of adding MELA dependency (I agree with all @jnyrup considerations here), transitive dependency management is one of the biggest PITA of Nuget and has even changed in .NET standard management.

As a style note, I would not check for if(m_logger != null). It should never be null, this is why also @jnyrup was suggesting a NoOpLogger, so enforce is to not be null.

My 2 cents A

[jnyrup]

In LogToDebugOrConsole the debug part is inside #if DEBUG and as we all consume a release mode of FluentFtp, it will effectively only be LogToConsole, i.e. no debugging part.

Instead of controlling logging via the knobs Config.LogFunctions etc. we could let the configuration of the logger decide what should be written.

protected void LogFunc(string function, object[] args = null) {
    if (m_logger.IsEnabled(LogLevel.Debug)) {
        ...
    }
}

For example a modern C# application will read the loglevel from one of these json files depending on the application is deployed to production or running locally while developing.

appSettings.production.json

{
  "Logging": {
    "LogLevel": {
      "Default": "Information",
      "FluentFTP": "Warning"
    }
  }
}

appSettings.Development.json

{
  "Logging": {
    "LogLevel": {
      "Default": "Information",
      "FluentFTP": "Trace"
    }
  }
}

Similarly there is LogLevel.Trace for Ip/UserName/Password.

LogLevel.Trace

Logs that contain the most detailed messages. These messages may contain sensitive application data. These messages are disabled by default and should never be enabled in a production environment.

Suggestion: Let m_logger default to a NoOpLogger instance, which does nothing. Then you don't have to null check m_logger, except in the setter of the property Logger.

[robinrodricks]

Instead of controlling logging via the knobs Config.LogFunctions etc. we could let the configuration of the logger decide what should be written

LogFunctions can be removed, we can unconditionally always log it. Unless you have a better way?

if (m_logger.IsEnabled(LogLevel.Debug)) {

Is this the industry standard way of doing it? If so I will support it like this.

Similarly there is LogLevel.Trace for Ip/UserName/Password

I don't mind removing the properties LogUser/Pass/IP, can you show a code example of how it would work? Like this?

if (m_logger.IsEnabled(LogLevel.Trace)) {
   show IP/user/pass
}else{
  hide it
}

[Adhara3]

In LogToDebugOrConsole the debug part is inside #if DEBUG and as we all consume a release mode of FluentFtp, it will effectively only be LogToConsole, i.e. no debugging part.

Probably #if DEBUG becomes handy maybe when running tests, I was just saying that code can be cleaned up a bit by injecting a dedicated logger when debugging.

Instead of controlling logging via the knobs Config.LogFunctions etc. we could let the configuration of the logger decide what should be written.

This would simplify but requires to bound function call logs to debug level. This is fine, I'm not against it, but @robinrodricks could keep the "business" knobs (function log etc...) as a separate control. E.g. you may keep a flag to obfuscate passwords, if set you replace password with "****", this kind of setting is more like an interceptor/manipulator than a logger itself and as such cannot be replaced by a Trace or Debug level. From an architecture point of view log request --> interceptor --> actual logger with level If you end up with something like that, I suggest defining an IInternalLoggerInterface, with higher level methods

internal interface IInternalLoggerInterface
{
    void Credentials(string username, string password);
    void Address(IPEndPoint address);
    void Message(string message);
}

This would allow you to keep all the log logic in one place instead of having if(Config.LogPasswords) all around

To me the important thing is not duplicating output knobs, ILogger decides where (console, file, database, UI, ...)

[jnyrup]

My comment was (probably) also meant as question/comment on why having all these logging knobs i FluentFTP. My line of thought is: FluentFtp has a piece of information and decides the severity (log level) of it. It's then up to the logger whether, where and how to log it.

if (m_logger.IsEnabled(LogLevel.Debug)) {

This is not strictly necessary (I also had to look this up again), it just lets you skip some logic if no logging will be performed anyways.

In the former ItemsToString() is always invoked, while in the latter it is only invoked if debug logging is enabled.

m_logger.LogInformation(function, args.ItemsToString());

if (m_logger.IsEnabled(LogLevel.Debug))
    m_logger.LogDebug(function, args.ItemsToString());

[robinrodricks]

@Adhara3 @jnyrup so can you guys decide what log settings you want or don't want (username/password/IP). I'm fine either way.

I would have said that if (m_logger.IsEnabled(LogLevel.Trace)) is an elegant way to replace username/password/IP. Alternatively, we can ALWAYS mask out sensitive info as I don't see why on earth anyone would WANT to add this into their logs. Its also against security best practices to log sensitive info.

And we could have used if (m_logger.IsEnabled(LogLevel.Debug)) to replace LogFunctions.

[robinrodricks]

So my final suggestion is:

• Remove LogFunctions (always log it)
• Remove LogUser/Pass/IP (Always mask out sensitive info)

[Adhara3]

I'm fine with functions mapped to debug and user/pass/IP mapped as trace. I still recommend merging this in a single place, after all this category mapping is a logic and it should be easy to change/intercept.

A

[robinrodricks]

Done:

• Remove logging settings as they are always enabled: LogFunctions
• Remove logging settings as sensitive data is always masked: LogIP, LogUserName, LogPassword

[robinrodricks]

What should we do about the 100 constructors? Can I only keep 1 or 2 of them because this is honestly excessive. Or should we keep it to allow "flexible usage"?

• FtpClient()
• FtpClient(string host)
• FtpClient(string host, NetworkCredential credentials)
• FtpClient(string host, int port, NetworkCredential credentials)
• FtpClient(string host, string user, string pass)
• FtpClient(string host, string user, string pass, string account)
• FtpClient(string host, int port, string user, string pass)
• FtpClient(string host, int port, string user, string pass, string account)
• FtpClient(Uri host)
• FtpClient(Uri host, NetworkCredential credentials)
• FtpClient(Uri host, string user, string pass)
• FtpClient(Uri host, string user, string pass, string account)
• FtpClient(Uri host, int port, string user, string pass)
• FtpClient(Uri host, int port, string user, string pass, string account)

[Adhara3]

Required stuff should be in the constructor only, not as a public property with setter (e.g. Host) This should allow the parameterless constructor to be deleted and is also a good practice.

Ideally

• FtpClient(Uri host)
• FtpClient(Uri host, NetworkCredential credentials)

should be enough with a couple of notes:

• Internally you put account into NetworkCredential domain field, which may not be standard (I'm no expert here), so that could be tricky for the user
• You can provide, if you wish, external helper methods (e.g. to go from host to Uri), basically migrating constructors to helper methods. The advantage is that you better separate responsibilities. Up to you.
• To me, properties that are constructor based should be readonly.

My 2 cents A

[robinrodricks]

To me, properties that are constructor based should be readonly.

Good point. In that case why not remove ALL the parametered constructors and just keep:

new FtpClient(ILogger logger = null, FtpConfig config = null)

Which kinda advertises the new features and allows users to submit the full config profile if they want.

Alternatively, because even Logger is a property:

• new FtpClient()
• new FtpClient(FtpConfig config)

And for migration, users can just:

new FtpClient(){ Host = XYZ, Credentials = XYZ }

[Adhara3]

n that case why not remove ALL the parametered constructors and just keep

The difference is that using the constructor you are sure that the object itself is usable, while using properties you are not.

var brokenClient = new FTPClient();
// brokenClient is in a broken state

var okClient = new FTPClient("host", "user", "password");
/* 
  ok client has all the required parameter, so it's not in a weird state. 
  Of course, this does not mean that "host" exists but there is nonetheless 
  a huge difference between "I require a host you did not provide" and 
  "the provided host does not exist"
*/

As a user (but also as a tech guy) the object coming from a constructor should be able to work without any further adjustment, or if you wish, required parameter should go into the constructor, optional one can be properties.

Another option is to keep the parameterless constructor as you suggest but make it internal and provide a FTPClientBuilder to the user which is a sort of guided way to create a ready to go client, a fancy way to separate required from optional stuff. It could be fluent, something like

var client = FTPClientBuilder
  .WithHost(...)
  .WithCredentials(...)
  .WithOptions(...)
  .Build();

My 2 cents A

[robinrodricks]

Done:

• Constructor API
  • New: 3 new FTP client constructors that accept FTP host, credentials, config and logger
  • Remove extranous constructors because properties can be used instead

[robinrodricks]

I think I'm ready to release this.

@Adhara3 @jnyrup @FanDjango Can you please test the latest master branch locally and see if it looks good?

I will release after at least 1 person confirms everything works fine.

[jnyrup]

I won't have time to test this until tomorrow evening at earliest. Can you publish a v40 beta version to nuget? That would make it easier for more to test and come with feedback before this large change.

[robinrodricks]

I won't have time to test this until tomorrow evening at earliest. Can you publish a v40 beta version to nuget? That would make it easier for more to test and come with feedback before this large change.

Test anytime you want. I have no idea how to publish a beta version to nuget :)

[FanDjango]

It is a pity you have removed FtpClient(host, port, user, pass...)

[FanDjango]

I needed to recode a few places to .Config....,

I have not set up logging yet,

For some reason in my test application I needed to add using FluentFTP.Client.BaseClient; (haven't researched why yet...)

so apart from progress messages, which I myself produce per Console.Writeln, I am running blind 👍 but my test suite for z/OS runs and finishes with no errors or problems.

I will release after at least 1 person confirms everything works fine

everything ?????? No, I can only confirm my special stuff. But yolt, right?

Ähm, release V40? Ok, but what about the Wiki? I needed to do quite a few changes and the logging changes will keep me busy some more time. Real world users who have not been following this thread will be slightly surprised at the build errors they will be getting... One could use a small dictionary of what compiler errors to handle in what way to fix it.

Update Logging to console enabled in client.Config. I can see what is happening and my test suite runs fine.

Update Implemented a custom loggerProvider for further tests

[jnyrup]

Test anytime you want. I have no idea how to publish a beta version to nuget :)

By setting the Version to e.g. 40.0.0-beta1. https://github.com/robinrodricks/FluentFTP/blob/f59e510ef75bab29881afd5e8b90ee2e19da0163/FluentFTP/FluentFTP.csproj#L31 https://docs.microsoft.com/en-us/nuget/create-packages/prerelease-packages

[FanDjango]

I will use a custom logger provider and some glue logic to get the log output to the status windows of my z/OS FTP client. Doing this cleanly exceeds the time I have in the next few days.

My summary of the experiences with the transition from V39.x to V40:

Up to now all my stuff works.

Some code modifications are needed, as you can imagine from your changes. It would be nice for these to be summarised in some migration text, mabye.

Especially those not experienced in implementing Ilogger (I am just a C# novice) could use a sample and if I have time I will provide mine (which will be really minimal, not like the ones to be found on the internet). It will enable a quick get up and go for those who need to migrate to V40.

[FanDjango]

For those follwing this thread and wanting to work with V40 as soon as possible with minimal disruption:

Either just use .Config.LogToConsole = true; on the client, or if you need some more logic, see below:

You need using Microsoft.Extensions.Logging;

and the coresponding Nuget packages...

and then (my sort of minimum approach, for a quick start):

		public class CustomConsoleLogger : ILogger {
			public CustomConsoleLogger() {
			}

			public IDisposable BeginScope<TState>(TState state) {
				return null;
			}

			public bool IsEnabled(LogLevel logLevel) {
				return true;
			}

			public void Log<TState>(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func<TState, Exception, string> formatter) {
				// Console.WriteLine($"{logLevel.ToString()} - {eventId.Id} - {formatter(state, exception)}");
				Console.WriteLine($"{formatter(state, exception)}");
			}
		}
		public class CustomLoggerProvider : ILoggerProvider {
			public CustomLoggerProvider() { 
			}

			public ILogger CreateLogger(string name) {
				return new CustomConsoleLogger();
			}

			public void Dispose() {
			}
		}

and

			// Either:

	/* */	// FTP_Sess.Config.LogToConsole = true;

			// Or:

	/* */	ILoggerFactory loggerFactory = LoggerFactory.Create(builder =>
	/* */	{
	/* */		//builder.AddConsole();
	/* */		//builder.AddDebug();
	/* */	});
        /* */
        /* */	loggerFactory.AddProvider(new CustomLoggerProvider());
        /* */
        /* */	ILogger<BaseFtpClient> logger = loggerFactory.CreateLogger<BaseFtpClient>();
        /* */
	/* */	FTP_Sess.Logger = logger;

will get you going if you need console output or logging, or putting the output where you need it, like a message panel or whatever..

I realize that this is old well known stuff for the C# experts out there but duh! Not for me, it wasn't.

[FanDjango]

And here is a bug:

File Listings appear correctly if you use FTP_Sess.Config.LogToConsole = true;

File Listings do not appear if you use an Ilogger implementation.

[robinrodricks]

@FanDjango Firstly thanks for testing and for the quick feedback.

Up to now all my stuff works.

Yes I intentionally did not change any of the FTP logic.

File Listings do not appear if you use an Ilogger implementation.

Sounds complicated. Is the ILogger crashing and aborting the file listing? We will have to fix this bug before releasing.

It is a pity you have removed FtpClient(host, port, user, pass...)

Good point, added port back into the constructor. I added it at the end if that's ok?

Some code modifications are needed, as you can imagine from your changes. It would be nice for these to be summarised in some migration text, mabye.

Yes I am planning on writing a migration guide for users shifting from pre-40 to v40.

[robinrodricks]

@FanDjango

https://github.com/robinrodricks/FluentFTP/wiki/v40-Migration-Guide

[robinrodricks]

@jnyrup @FanDjango Released a beta version!

https://www.nuget.org/packages/FluentFTP/40.0.0-beta1

[FanDjango]

@robinrodricks Re. this:

File Listings appear correctly if you use FTP_Sess.Config.LogToConsole = true;

File Listings do not appear if you use an Ilogger implementation.

File Listings and other sundry output are sent by you via:

				case FtpTraceLevel.Verbose:
					m_logger.Debug(message);
					break;

For .Debug (and .Trace) to be handled, you need an appsettings.json or even an appsettings.development.json file to configure ILogger.

This is not really attractive.

I changed it to

				case FtpTraceLevel.Verbose:
					m_logger.LogInformation(message);
					break;

and I am getting my messages.

You can take it onward from there, I suppose.

[FanDjango]

@robinrodricks

Since I want to discern between the old loglevels "Verbose" and "Info" in my application, I changed the code to;

				case FtpTraceLevel.Verbose:
					m_logger.LogInformation("+++ " + message);
					break;

But something has changed in the logging levels. If I output the loglevels together with the message, you can see some strange things:

Information - 0 - +++ Response: 220-FTPD1 IBM FTP CS V2R4 at bla.bla, 12:19:10 on 2022-08-29.
Information - 0 - Response: 220 Connection will close if idle for more than 5 minutes.
Information - 0 - Status:   Detected FTP server: IBMzOSFTP
Information - 0 - Command:  AUTH TLS
Information - 0 - Response: 534 Server setup for TLS failed
Information - 0 - Command:  USER ***
Information - 0 - Response: 331 Send password please.
Information - 0 - Command:  PASS ***
Information - 0 - Response: 230 GEEK is logged on.  Working directory is "GEEK.".
Information - 0 - Command:  FEAT
Information - 0 - +++ Response: 211- Extensions supported
Response: SIZE
Response: MDTM
Response: UTF8
Response: LANG en*
Response: AUTH TLS
Response: PBSZ
Response: PROT
Information - 0 - Response: 211 End
Information - 0 - Text encoding: System.Text.UTF8Encoding+UTF8EncodingSealed
Information - 0 - Command:  OPTS UTF8 ON
Information - 0 - Response: 501 command OPTS aborted -- no options supported for UTF8

Some Response: loggings are verbose (+++), some are info.

Also, look at the following log excerpt:

Information - 0 - +++ Disposing FtpSocketStream...
Information - 0 - CloseDataStream
Information - 0 - Response: 250 List completed successfully.
Information - 0 - +++ Disposing FtpSocketStream...
Information - 0 - +++ Confirmed format IBMzOS
Information - 0 - Command:  TYPE I
Information - 0 - Response: 200 Representation type is Image
Information - 0 - DownloadFile
Information - 0 - GetFileSize
Information - 0 - GetListing
Information - 0 - OpenPassiveDataStream
Information - 0 - Command:  EPSV
Information - 0 - Response: 229 Entering Extended Passive Mode (|||4639|)
Information - 0 - Connecting to ***:4639
Information - 0 - Command:  LIST /projects/test.bin
Information - 0 - Response: 125 List started OK
Information - 0 - +++ +---------------------------------------+
Information - 0 - +++ Listing:  -rw-r-----   1 GEEKS    SYS1      132480 Jan  2  2022 /projects/test.bin
Information - 0 - +++ -----------------------------------------
Information - 0 - +++ Disposing FtpSocketStream...

Why is confirmed format verbose? Why is Donload GetFileSize, GetListing etc. file not verbose?

I am confused that some has changed from before, somehow, maybe? This is breaking the coloring and filtering logic of my log output window in the application.

[robinrodricks]

Individual commands have not been changed. Whatever is the old log level has been preserved. Only thing has changed is the mapping from Ftp log level to MS log level.

Feel free to make changes to this and file a PR please.

[FanDjango]

I will investigate and then I will do that. Thanks.

[robinrodricks]

I will investigate and then I will do that. Thanks.

Thanks very much in advance for any fixes towards v40.

[FanDjango]

It is a pity you have removed FtpClient(host, port, user, pass...)

Good point, added port back into the constructor. I added it at the end if that's ok?

host, port, user, pass seems more logical, though

[robinrodricks]

host, port, user, pass seems more logical, though

That's true, however many people don't pass a port and just host/user/pass. Because most times FTP is on known ports.

[jnyrup]

The two FTP servers I connect to are both public and use default port 21, so with the new set of constructors I'll have change from

new FtpClient(host)

to

new FtpClient
{
    Host = host
}

Not a deal-breaker for me at all, but I would have gone with a single FtpClient(string host, FtpConfig config = null) since host is the minimal needed information to attempt to connect to an FTP server and config because it is passed along to BaseFtpClient. Credentials and a non-standard port are optional in my view.

[jnyrup]

The downside of making all properties optional is that one can construct an instance of FtpClient which is not ready to use.

var client = new FtpClient();
client.Connect();             // who to connect to?

[robinrodricks]

since host is the minimal needed information to attempt to connect to an FTP server

@jnyrup Fair enough. I have added a host constructor. Does it work for you?

public FtpClient(string host, int port = 0, FtpConfig config = null, ILogger logger = null)

The downside of making all properties optional

Agreed but I wanted to support a use-case where users just construct the client and store an obj ref and then configure it at a later point.

[Adhara3]

Agreed but I wanted to support a use-case where users just construct the client and store an obj ref and then configure it at a later point.

A factory method or a factory pattern should be used in this case. Storing broken objects it's an anti-pattern.

[Adhara3]

The downside of making all properties optional is that one can construct an instance of FtpClient which is not ready to use.

var client = new FtpClient();
client.Connect();             // who to connect to?

Sounds quite similar to this :)

[robinrodricks]

A factory method or a factory pattern should be used in this case. Storing broken objects it's an anti-pattern.

@Adhara3 I want to avoid over complicating the API. I really don't like java-style SOLID principles and the convoluted class structures that it produces. Besides this library is super simple, just one main class with a bunch of methods. So I'm trying to make it as simple to use as possible.

From day 1 my goal has been to keep it simple so I have an API like :

var result = client.DownloadFile(..)

Rather than:

var config = new SimpleFileTransferConfig { .... };
var downloader = new SimpleFileDownloader(client, config);
var outputStream = new FileStream(...)
var result = downloader.Start(outputStream);

I know it is debatable, but I just feel like if we over-implement SOLID principles there is no end in sight and we could have 50 classes where 1 would suffice. Which IMO is also an anti-pattern.

I agree that using a future-proof and extensible config object is probably something we need though:

var config = FileTransferConfig { ... };
var result = client.DownloadFile(path, config);

---

The downside of making all properties optional is that one can construct an instance of FtpClient which is not ready to use.

The thing is that the Connect method will throw an error if there is no host specified, so I wouldn't say the object is in a "broken" state, just that its not ready to use.

After all I have no idea how my thousands of users are actually using it. Maybe they are using a blank constructor and they have other methods to configure the client at a later point in time, who knows? I want to give people the freedom to use it however they want, like a rubber band. I don't want just ONE right way of using it and every other way is WRONG or restricted by the system.

[FanDjango]

I don't know if there is a "time-out" symbol or meme, but like in a basket-ball game, can I request such a time-out, or subroutine call?

I have a problem with V40. Well, I had some concerns and was saying, not a deal breaker, just like @jnyrup.

But my problem, reported by a (paying) user of my application, is: TLS AUTH not working (Test server is ProFTPD). Now, I used my local tests and they work (Console Application, new Client() etc. etc. AutoConnect and all is ok), the actual application though (WinForms) does not connect. The problem is certainly on my side, I think, but it is provoked by the migration V39->V40.

Now is too early to go into details but please be assured I have bisected and debugged to a certain degree and I am sure I will find the problem. But what I am saying is: Don't release yet. I am re-using the client for multiple sequential (connecting, disconnecting, etc.) connections and I know it is a no-no (has worked flawlessy up to now), but the failure is on the first usage. So, duh! Sadly there are a few other customer requests pending so I don't see myself debugging this further until next week begins, customer has rolled back to one of my apps that uses V39.

The concerns I have, independently of your discussions about the perfect way to present this library to a harried, hurried and anxious to use it user, is that my application suffers from:

The new logger or is it overkill?

1. I want an easy way to FTP(S) with the emphasis on the S, the normal stuff I could do myself. No time for the SSL, I thought.
2. I want the full conversation with the server to report to the user.
3. I don't want to package lots of .dlls other than FluentFTP (which I need to do if I implement ILogger)
4. I want to get all the messages I need without another stupid config file. The verbose ones are missing, see above.

Can you imagine? I have users of the app, and I am to a certain degree already considering cloning the latest 39.xx and using that, which means rolling back. I can do without the hassle.

[robinrodricks]

TLS AUTH not working

I'm sorry about that.

Don't release yet.

Take your time, when it is ready we will release.

So, duh! Sadly there are a few other customer requests pending so I don't see myself debugging this further until next week begins, customer has rolled back to one of my apps that uses V39.

Yes, yes, don't worry, I will release when it is good.

I don't want to package lots of .dlls other than FluentFTP (which I need to do if I implement ILogger)

Many users chose this (see convo above). I agreed with them on the premise that you can implement your own ILogger implementation and do anything you want with it. Plus we have never been able to integrate cleanly with log4net and other big .NET loggers and this was a goal too.

I don't want to package lots of .dlls

This is what the "new" .NET core/6+ system has become. Each MS library is now shipped as a separate nuget package, rather than being a single "framework" as such. It is the inevitable proliferation of package management.

I want the full conversation with the server to report to the user.

See post below.

I want to get all the messages I need without another stupid config file.

Agreed. Config files can become a deployment hassle.

I have users of the app, and I am to a certain degree already considering cloning the latest 39.xx and using that, which means rolling back.

Don't do this please, let us accommodate your use case.

[robinrodricks]

@FanDjango There was something wonky in the new logger implementation as I was not always adding a prefix to the messages. That could be why you noticed things going weird. I have now always added a prefix, just like the old system.

I have also added back client.LegacyLogger which is an easy way to get log messages. If you use this you don't need a config file. Please try it out:

Release: https://www.nuget.org/packages/FluentFTP/40.0.0-beta2

Docs: https://github.com/robinrodricks/FluentFTP/wiki/v40-Migration-Guide#renamed

[FanDjango]

I have also added back client.LegacyLogger which is an easy way to get log messages. If you use this you don't need a config file. Please try it out

Ok, in my fork, I had the same idea and did that already, but I am using your newest master now and your LegacyLogger works fine. One note though: If using LegacyLogger in a Consoe Application you must turn off Config.LogToConsole depending on your needs.