search

Nikey646 / VndbSharp

A C# Vndb API Library. #OriginalNamingScheme
MIT License
18 stars 4 forks source link

Version 2 Discussion #71

Closed Nikey646 closed 2 years ago

Nikey646 commented 4 years ago

I'm currently considering re-writing VndbSharp in .Net 5, focusing heavily on the feature that C# 9 provides.

This could block building for lower versions of .Net, EG: .Net Core or .Net Standard, depending on how well C# 9 plays with it. (Changes that require runtime modifications means that anything below .Net 5 will not be supported).

The current main changes I'm looking to implement are

Connection Pooling

The idea is that VndbSharp would maintain a configurable amount of TCP Sockets in either an Initialized or Uninitialized state. When you use a command, (EG: GetVisualNovelAsync), internally it would request a connection from the pool, which would be the TCP Socket.

Before being given out from the pool, it would launch the login command making it into an Initialized TCP Socket / connection. This connection would then remain open, until either the VndbSharp primary class was disposed of, or if I can figure out a nice implementation, a configurable (probably minutes) timeout period, where the socket will be disposed off.

After the command has finished executing and returning, it would then be returned to the connection pool to be used in the future.

The primarily benefit of this feature is that because the Vndb API Server can only be used on a single pipe, while it is incredibly quick you cannot make multiple API requests without waiting for them to finish in order.

Tuple/OneOf return type for methods

The idea of this is to greatly improve the error flow handling of the application. Because Exception Control Flow is an Anti Pattern I have attempted to avoid that at all costs.

By returning Tuples, the usage would then be something like 

var (result, error) = await vndb.GetVisualNovelAsync(...);
if (error != null) 
{
    // Handle errors
    return;
}

// Continue on as normal with result

Alternatively, using the package OneOf would be a bit more familiar, but has some downsides. The usage would be something like

var response = await vndb.GetVisualNovelAsync(...);
// T1 is the Error Information
if (response.TryPickT1(out var error))
{
    // Handle errors
    return;
}

var data = response.AsT0; // Returned data
// Continue on as normal with result

The most obvious downside would be that it's unclear from the code whether or not T0 is the data or the error returned by vndb. But one that isn't initially clear, is that the size of the OneOf library is actually rather huge, because it has to manually define OneOf<T0, ... T8> in just the base library, with more being provided in OneOf.Extended.

The benefits in just looking at how the control flow would work with either solution definitely warrant the signature change of the methods, since it removes the ugly null / .GetLastError() control flow currently required.

Records for the Models

This would mean rewriting every single Model currently inside the code, and possibly require modifications to how the Json Deserializing currently works.

The benefit from the user stand point of changing to Records, would mean that all the models become writable outside of VndbSharp, because they would be created with the new init; instead of private set;. Furthermore, because Records support the new C# 9 with-expressions, this again benefits the user in that models become mutable and the need to maintain a mirror of the VndbSharp Models just to modify the data before displaying it is removed.

One downside is that the models would go from a reference type to a value type. I don't actually know the full extend of this, to be honest.

A decision would need to be made in regards to which pattern would be used. Using positional records automatically adds a deconstructor, so grabbing just certain values could be done like var (id, name, ...) = record;, but with the amount of values that our models have, this might be excessive. Especially when declaring the record itself. EG: The full VisualNovel declaration would look like:

public record VisualNovel(UInt32 Id, String Name, String OriginalName, SimpleDate Released, ReadOnlyCollection<String> Languages, ReadOnlyCollection<String> OriginalLanguages,  ReadOnlyCollection<String> Platforms, ReadOnlyCollection<String> Aliases, VisualNovelLength? Length, String Description, VisualNovelLinks VisualNovelLinks, String Image, Boolean IsImageNsfw, ImageRating ImageRating, ReadOnlyCollection<AnimeMetadata> Anime, ReadOnlyCollection<VisualNovelRelation> Relations, ReadOnlyCollection<TagMetadata> Tags, Single Popularity, Double Rating, ReadOnlyCollection<ScreenshotMetadata> Screenshots, ReadOnlyCollection<StaffMetadata> Staff);

If to properly handle renaming the API json names to the C# record names it would require adding attributes in this pattern, it could get very messy quite quickly.

Switching to System.Text.Json

The System.Text.Json library seems to have matured quite a bit at this point, and offers performance on par if not better than Newtonsoft.Json.Net.

Using either System.Text.Json directly, or creating an IDeserializer interface that allows either System.Text.Json or Newtonsoft.Json.Net to be used to parse the API responses might be a viable solution.

Changing to System.Text.Json would also require figuring out how to handle all of the custom name differences, and custom converters used to coerce the data from VNDB into C# models.

Provide an implementation of the Vndb class wrapped in Polly.

By wrapping the methods inside of a Polly Retry policy, the client can automatically handle the Throttled error from the Vndb API client.

This should be provided by a second Nuget Package, and the implementation would most likely be a class that has a private instance of the Vndb class. Methods that don't go to the Vndb API will just be passed through to the main Vndb class, while all other methods will be wrapped in the Retry Policy.

Research required to determine if the Retry Policy can be configured to wait based off of the Throttle error's recommended wait times. If it can, should the minimum or maximum wait times be used? If it can't, how long should each retry wait? Should it be exponential?

Nikey646 commented 4 years ago

@micah686 @Mithnar Do you guys have any suggestions for Version 2, or comments on what I've currently posted here?

micah686 commented 4 years ago

Without looking through all of the VndbSharp code again, here are my suggestions:

Also, is this work going to be done on the existing code, or are you going to build it from the ground up? You should also create a new branch for this effort, probably an orphaned branch, so you don't carry over any history, maybe named "v2" or something.

Are you going to make the 2 versions compatible with each other syntax wise, or will you have 2 versions? If 2 versions are going to be created (v1 and v2), I think it would to good mark the version 1 as "legacy", so as mark that the user should upgrade going forward. I'm of the opinion(and you might not agree with me, which is fine), that maybe version 2 should be a .net core/.NET 5 version only, and maybe leave off the old .net framework support going forward. Anyone that would need to use the .net framework could use the legacy version, but where projects moving forward should use the modern versions.

Nikey646 commented 4 years ago

Maybe create a (generic/abstract?) Filter for the VndbFilter class, as it seems like a fair amount of the filter code is repeated, and could be shrunk down

Would you believe me if I told you the code would be 3-4 times longer if I hadn't already had an abstract filter?

The code that repeats a lot is the bare minimum that needs to be configured to make each filter statically typed to what vndb supports for that filter itself. For example, Alias Id filter only supports equal, but a generic id filter supports more. (Even more fun is that Alias Id / Id uses id=, while User Id uses uid= lol...)

Maybe see about combining some methods, as I feel like some functions could be merged together.

What functions? If it's viable, I'm open to doing it. However as far as Get / Set functions go, they're either overloaded to provide optional arguments as per what the API supports (for set functions), while Get functions are unique per data structure returned.

Perhaps have a built in retry/wait function for any throttled error that the user could enable. That way, if a response gets Throttled, it could retry up to ~3 times with after delays of 120seconds each? Just something where the user wouldn't have to potentially deal with a throttled error.

With the new return types, it should be relatively easy to wrap the vndb client with Polly on a user level. To make this more user friendly, we could put the vndb client to an interface, and create a separate library that wraps all vndb client methods that send a request in a Polly Retry and Wait policy. The biggest issue that I can see from using Polly is that there would need to be assumptions about how long the timeout needs to be, rather than respecting the timeout duration provided by the API. Further research might make this assumption invalid.

I know I have brought this up before, and it's not an easy issue to deal with, but allow logging in with username/pass without having to compile the library with the #UserAuth flag. As I suggested before, maybe require a string to be sent acknowledging the downfalls of SecureString. This is somewhat important, as people who download the library from nuget wouldn't be able to use any of the set commands without recompiling it themselves, and potentially maintaining a local copy of VndbSharp.

My stance remains on this, we need a secure way that puts the developer in the know about the issues SecureString represents.

The Set* methods will most likely be under a similar flag in Version 2, so they don't appear under Intellisense from the Nuget package.

An idea I just thought of, since the client is designed to be extended to add missing features, might be to simply require the user to extend the main vndb client and add a method to set protected username and password properties. This means the properties are not exposed publicly on the vndb client, but internally the methods can check to see if they're set. This could also be done by reflection, if they want to go that way.

Also, is this work going to be done on the existing code, or are you going to build it from the ground up? You should also create a new branch for this effort, probably an orphaned branch, so you don't carry over any history, maybe named "v2" or something.

It'll most likely be a ground up, take the existing parts that aren't being heavily rewritten (Pretty much just the filters and some misc code at this point). I'm not sure if I'll sit down and smash it out in a day or two, or if it'll be done over weeks, so creating a branch will depend on how much progress I make when I start this.

Are you going to make the 2 versions compatible with each other syntax wise, or will you have 2 versions?

I'm not sure what you mean by syntax? Do you mean method names? If so, the names will most likely remain the same but the return results will definitely be different, so they will not be API compatible. This is why it will be Version 2, because I'm loosely following SemVer.

If 2 versions are going to be created (v1 and v2), I think it would to good mark the version 1 as "legacy", so as mark that the user should upgrade going forward.

v1 is what is currently released and available on Nuget. The library will just be released under 2.0.0 for the initial release in the same Nuget Package. Nuget has no limits on how many versions you can upload, and in fact it's not possible to delete a package version.

I'm of the opinion(and you might not agree with me, which is fine), that maybe version 2 should be a .net core/.NET 5 version only, and maybe leave off the old .net framework support going forward.

This will depend on how the C# 9 features work with .Net Standard 2.0. In the event it just requires the .Net 5 or higher SDK to compile, I see no reason to avoid having the library compatible with .Net Standard 2.0, and there for... .Net Core 2.0+, .Net Framework 4.6.1+, Mono 5.4+, Xamarin iOS 10.14+, Xamarin Mac 3.8+, Xamarin Android 8.0+, UWP 10.0.16299+, and Unity 2018.1+.

There is literally no downside to allowing the new version of the library to be used on these platforms, if C# 9 compiled code works on there. They will not be able to use the source directly, without upgrading to a .Net 5 SDK compatible version, but the released binary will work fine, which is the way .Net Standard was designed to work.

Anyone that would need to use the .net framework could use the legacy version, but where projects moving forward should use the modern versions.

This is how the Version 1 release currently works. It provide .Net Standard 1.3 version which is what anything besides .Net Framework targets. Most likely if you do not use the exact .Net Framework version provided, you will actually end up using the .Net Standard 1.3 build.

Targeting a lower "version" of .Net Standard doesn't mean we are restricting the functionality of the library, it simply means that version of .Net Standard supports the functionality required for VndbSharp.

micah686 commented 4 years ago

1) I knew that it had an abstract filter, I was just wondering if it could be condensed any more 2) No comments 3) I haven't messed with Polly much, but could it have default timeout values that the user could override? 4) I do like your idea of requiring the programmer(s) to extend the UserAuth methods/functions if they need to use them. That should strike a good balance, since the programmer needs to know about the risks when extending the method, but not requiring the programmer(s) to have a seperate copy of the library in order to use the Set methods. 5) No comments 6) I mean the syntax and parameters of the methods, so instead of something like client.GetVisualNovelAsync(VndbFilters.Id.Equals(vnid), VndbFlags.FullVisualNovel), it would be something like client.GetVisualNovelAsync(IdFilter.Equals(vnid), VndbFlags.FullVisualNovel) 7) no comments 8) no comments 9) no comments

Nikey646 commented 4 years ago
  1. I knew that it had an abstract filter, I was just wondering if it could be condensed any more

I don't believe so.

  1. I haven't messed with Polly much, but could it have default timeout values that the user could override?

Not as far as I'm aware, unless the end user sets it up by themselves. Again, further research might change the answer to this one, and the Polly implementation would be a secondary package that would create a PollyVndb class that implements IVndbClient (or similarly named interface), and internally just refer to a private copy of Vndb with all methods wrapped in the appropriate Polly methods. This could be done by users right now, I believe.

tl;dr: Polly will be optional, not required. The main VndbSharp package will not use Polly.

  1. I mean the syntax and parameters of the methods, so instead of something like client.GetVisualNovelAsync(VndbFilters.Id.Equals(vnid), VndbFlags.FullVisualNovel), it would be something like client.GetVisualNovelAsync(IdFilter.Equals(vnid), VndbFlags.FullVisualNovel)

All the filter classes are a child class of the VndbFilters class to make finding what is available in Intellisense much easier. Moving them out of the VndbFilters class would be counter productive to that.

If you wanted to have the names shortened for your own personal usage, with your using VndbSharp.*; statements, you could put

using IdFilter = VndbSharp.VndbFilters.Id;

Which would allow you to use the filter as you suggested. IdFilter.Equals(vnId).

Nikey646 commented 4 years ago

What are your thoughts on doing the models like this, instead of having a record per file with a property per line like what we currently do?

https://github.com/Nikey646/VndbSharp/blob/937e47b35c144d0d299225e85167745f82f1a11a/src/VndbSharp/Models/VisualNovels.cs#L1-L30

The only issue is documentation, I'm not sure how that will work.

As far as handling the differences in names for json, if we go with System.Text.Json, it looks like we'll need to run a Converter per Record to adjust names, which shouldn't be too terrible. Not the most efficient or nice looking, but the JsonPropertyName attribute doesn't work on record properties when they are setup in this fashion.

micah686 commented 4 years ago

I haven't looked too much into record types, but I think that the example you gave is too "busy". I think it would be better to have init properties, since that would help with readability. I think that having all of the properties of the VisualNovel inline might be too much, if someone needs to debug the VisualNovel record.

[EDIT] I would be fine with the new record types, but I think readability might be an issue. So instead of having the records like:

public record VisualNovel(UInt32 Id, String EnglishTitle, String OriginalTitle, SimpleDate Released, 
         IReadOnlyCollection<String> Languages, IReadOnlyCollection<String> InitialReleaseLanguages, 
         IReadOnlyCollection<String> Platforms, IReadOnlyCollection<String> Aliases, VisualNovelLength? Length, 
         String Description, VisualNovelLinks VisualNovelLinks, String Image, ImageRating CoverImageRating, 
         IReadOnlyCollection<AnimeMetadata> Anime, IReadOnlyCollection<VisualNovelRelation> Relations, 
         IReadOnlyCollection<TagMetadata> Tags, Single Popularity, Single Rating, 
         IReadOnlyCollection<ScreenshotMetadata> Screenshots, IReadOnlyCollection<StaffMetadata> Staff);

you could do something like this:

public record VisualNovel(UInt32 Id,
         String EnglishTitle, 
         String OriginalTitle, SimpleDate Released, 
         IReadOnlyCollection<String> Languages,
         IReadOnlyCollection<String> InitialReleaseLanguages, 
         IReadOnlyCollection<String> Platforms,
         IReadOnlyCollection<String> Aliases,
         VisualNovelLength? Length, String Description,
         VisualNovelLinks VisualNovelLinks, String Image,
         ImageRating CoverImageRating, 
         IReadOnlyCollection<AnimeMetadata> Anime,
         IReadOnlyCollection<VisualNovelRelation> Relations, 
         IReadOnlyCollection<TagMetadata> Tags,
         Single Popularity, Single Rating, 
         IReadOnlyCollection<ScreenshotMetadata> Screenshots,
         IReadOnlyCollection<StaffMetadata> Staff);
micah686 commented 3 years ago

Also, I have a suggestion for a feature, but I'm not sure if it should go on VndbSharp or not. What do you think of having methods/functions that could be used to get the parent tag/trait of a given tag/trait?(Especially a way to get the top-most parent tag/trait). I think it would be helpful to have something like GetParentTag(Tag childTag) and GetRootParentTag(Tag childTag) in order to make it easier to find the parent(and possibly child elements) of tags/traits.

Or do you think this should be implemented on the client side?

Nikey646 commented 3 years ago

Also, I have a suggestion for a feature, but I'm not sure if it should go on VndbSharp or not. What do you think of having methods/functions that could be used to get the parent tag/trait of a given tag/trait?(Especially a way to get the top-most parent tag/trait). I think it would be helpful to have something like GetParentTag(Tag childTag) and GetRootParentTag(Tag childTag) in order to make it easier to find the parent(and possibly child elements) of tags/traits.

Or do you think this should be implemented on the client side?

I was thinking of adding support for Tags/Traits into Version 2. The biggest issue would be how to "store" these, I would probably just use Microsoft.Extensions.Caching library unless you have a better suggestion.

As far as implementing the feature, I would love to have it as a property, but I don't think that would be possible, sadly.

micah686 commented 3 years ago

Also, I have a suggestion for a feature, but I'm not sure if it should go on VndbSharp or not. What do you think of having methods/functions that could be used to get the parent tag/trait of a given tag/trait?(Especially a way to get the top-most parent tag/trait). I think it would be helpful to have something like GetParentTag(Tag childTag) and GetRootParentTag(Tag childTag) in order to make it easier to find the parent(and possibly child elements) of tags/traits.

Or do you think this should be implemented on the client side?

I was thinking of adding support for Tags/Traits into Version 2. The biggest issue would be how to "store" these, I would probably just use Microsoft.Extensions.Caching library unless you have a better suggestion.

As far as implementing the feature, I would love to have it as a property, but I don't think that would be possible, sadly.

When you say "store", do you mean the trait/tag dump? Also, you would love to have what as a property?

Nikey646 commented 3 years ago

Also, I have a suggestion for a feature, but I'm not sure if it should go on VndbSharp or not. What do you think of having methods/functions that could be used to get the parent tag/trait of a given tag/trait?(Especially a way to get the top-most parent tag/trait). I think it would be helpful to have something like GetParentTag(Tag childTag) and GetRootParentTag(Tag childTag) in order to make it easier to find the parent(and possibly child elements) of tags/traits. Or do you think this should be implemented on the client side?

I was thinking of adding support for Tags/Traits into Version 2. The biggest issue would be how to "store" these, I would probably just use Microsoft.Extensions.Caching library unless you have a better suggestion. As far as implementing the feature, I would love to have it as a property, but I don't think that would be possible, sadly.

When you say "store", do you mean the trait/tag dump? Also, you would love to have what as a property?

Yeah, the actual dump data that would be used. Version 2 will probably handle the data rather than just providing the models and a helper method to download them.

A "ParentTag/Trait" property. So accessing it would have been tag.Parent.Name

micah686 commented 3 years ago

Here's an example that might be useful for getting the parent tags/traits: https://github.com/Onkelsam/VNDBUpdater/blob/92c7039351e7ba86daee5087f73225ba8f956460/VNDBUpdater/Services/Traits/TraitService.cs

Nikey646 commented 3 years ago

I would prefer the Parent trait/tag was fetched on demand, rather than looping over the entire collection every time the dumps are loaded. It's entirely possible that the Parent trait/tag will never be used as well.

micah686 commented 3 years ago

Yeah, I know. It was more of an example for you to use as reference.

So, what features of v2 are higher priority to work on, so I can maybe start working on some of it?

Nikey646 commented 3 years ago

Yeah, I know. It was more of an example for you to use as reference.

So, what features of v2 are higher priority to work on, so I can maybe start working on some of it?

The models/records would need to be defined first for what you could contribute. However I still need to setup utilities like Stylecop.

Nikey646 commented 3 years ago

The only issue is [records] documentation, I'm not sure how that will work.

Blocked by https://github.com/dotnet/roslyn/pull/49134

Nikey646 commented 2 years ago

Closing as per lack of development on library.

Current master branch is to be considered the primary and only branch with only bug fixes going forward unless renewed interested in the library.