Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP fix/implement caching upstream #93 #102

Closed
wants to merge 1 commit into from

Conversation

tomzo
Copy link
Contributor

@tomzo tomzo commented Sep 28, 2018

For read-through caching, queries to registration and package service
must be directed to upstream server. Otherwise client would get always
404 because there is nothing in the cache to begin with.
We can add caching queries too, but that requires integrating catalog
reader to refresh responses for already cached packages.

What does this PR do?

Fixes read-through cache.

Closes Issue(s)

#93

Additional Notes

  • still need to add an option to parametrrize cache source. I don't think previous option was ever sufficient "PackageSource": "https://api.nuget.org/v3-flatcontainer/". I think we need to specify full URL to the index of nuget server. Such as "https://api.nuget.org/v3/index.json".
  • this is working now, but there are 3 endpoints which work by checking local packages first and then fallback to ask nuget.org. I think it would be more sensible if caching endpoint was separate from private index. Please tell me if I can move it.
  • I'll add unit tests before merge.

For read-through caching, queries to registration and package service
must be directed to upstream server. Otherwise client would get always
404 because there is nothing in the cache to begin with.
We can add caching queries too, but that requires integrating catalog
reader to refresh responses for already cached packages.
@@ -9,6 +11,19 @@ namespace BaGet.Core.Mirror
/// </summary>
public class FakeMirrorService : IMirrorService
{
Task<IReadOnlyList<string>> emptyVersions = Task.Factory.StartNew(() => new List<string>() as IReadOnlyList<string>);
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of Task.Factory.StartNew, how about Task.FromResult?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.

@loic-sharma
Copy link
Owner

loic-sharma commented Sep 28, 2018

Thank you for tackling this! I think your current solution has a flaw. Say package Test has two versions: v1.0.0 and v2.0.0. This behavior seems confusing:

  1. I setup a clean installation of BaGet
  2. I browse v3/registration/test/index.json and see both versions as expected
  3. I browse v3/registration/test/2.0.0.json, package 2.0.0 gets mirrored
  4. I browse v3/registration/test/index.json and see just version 2.0.0. Version 1.0.0 is missing!

This is a tough problem to solve. A possible solution may be to always contact the upstream source. This may be feasible as the NuGet client caches HTTP responses. What're your thoughts @tomzo?

HasReadme = false; //
IconUrl = NullSafeToString(package.IconUrl);
Language = null; //
LicenseUrl = NullSafeToString(package.LicenseUrl);
Copy link
Owner

@loic-sharma loic-sharma Sep 28, 2018

Choose a reason for hiding this comment

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

What about just package.LicenseUrl?.ToString();?

@tomzo
Copy link
Contributor Author

tomzo commented Sep 28, 2018

This is a tough problem to solve. What're your thoughts @tomzo?

Second endpoint would solve that too.
Because currently this happened:

  • in 1. query to db returned none, so we fallback to mirror.
  • in 4. you got response based on non-empty reply from query to db, which now contains package which is treated same as private one.
    If we don't split private/public cached packages, that each reply will have to also check with upstream to see what other packages are available.

@tomzo
Copy link
Contributor Author

tomzo commented Oct 12, 2018

I have implemented caching with new endpoint in my fork, which solves above problem. Cached packages are not indexed in database at all, we just query nuget.org.
I can submit a PR once #108 one is merged.

@tomzo tomzo closed this Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants