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

NuGetV2-OData: retrieve versions in descending order #2008

Merged
merged 1 commit into from Nov 10, 2016

Conversation

3 participants
@cdrnet
Member

cdrnet commented Nov 8, 2016

Some of the NuGet servers have broken feed paging. This PR changes the order such that newer packages are returned first (Publish date, descending). In case of broken paging, getting only newer versions seems less of an issue than getting only older versions. This is in particular relevant when using the update command without explicitly specifying a version (which downgrades to older versions otherwise). With this change, the request is also closer to how the NuGet Package Explorer queries all versions of a package.

NuGetV2-OData: retrieve versions in descending order
Some feeds have broken paging behavior. Getting only newer versions seems less of an issue than getting only older versions.
@cdrnet

This comment has been minimized.

Show comment
Hide comment
@cdrnet

cdrnet Nov 8, 2016

Member

Verified against Artifactory.

Member

cdrnet commented Nov 8, 2016

Verified against Artifactory.

@cdrnet

This comment has been minimized.

Show comment
Hide comment
@cdrnet

cdrnet Nov 9, 2016

Member

@forki have we had issues with other feeds (e.g. proget) in the past where this change could potentially cause problems? (If so, I'd expect NuGet Package Explorer to not work with them either)

Member

cdrnet commented Nov 9, 2016

@forki have we had issues with other feeds (e.g. proget) in the past where this change could potentially cause problems? (If so, I'd expect NuGet Package Explorer to not work with them either)

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 9, 2016

Member

I never tried that. We can try it and if things break then we restrict this change to artifactory

Member

forki commented Nov 9, 2016

I never tried that. We can try it and if things break then we restrict this change to artifactory

@cdrnet

This comment has been minimized.

Show comment
Hide comment
@cdrnet

cdrnet Nov 9, 2016

Member

Does Paket already understand what server it is talking to?

Member

cdrnet commented Nov 9, 2016

Does Paket already understand what server it is talking to?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 9, 2016

Member

We only know the url

Am 09.11.2016 09:02 schrieb "Christoph Ruegg" notifications@github.com:

Does Paket already understand what server it is talking to?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2008 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNHTWQVWj6jT202uN9Zq_5nB02YJDks5q8X4pgaJpZM4KsNLB
.

Member

forki commented Nov 9, 2016

We only know the url

Am 09.11.2016 09:02 schrieb "Christoph Ruegg" notifications@github.com:

Does Paket already understand what server it is talking to?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2008 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNHTWQVWj6jT202uN9Zq_5nB02YJDks5q8X4pgaJpZM4KsNLB
.

@cdrnet

This comment has been minimized.

Show comment
Hide comment
@cdrnet

cdrnet Nov 9, 2016

Member

If we ever need it (which I hope not), it actually tells so in the HTTP header:

HTTP/1.1 200 OK
Server: Artifactory/4.10.0     // it's actually 4.14.1 #wtf
DataServiceVersion: 2.0
Content-Type: application/atom+xml;charset=utf-8
...

Technically we could leverage this to skip the v3 trials there (which it does not support), but let's not complicate things unnecessarily...

Member

cdrnet commented Nov 9, 2016

If we ever need it (which I hope not), it actually tells so in the HTTP header:

HTTP/1.1 200 OK
Server: Artifactory/4.10.0     // it's actually 4.14.1 #wtf
DataServiceVersion: 2.0
Content-Type: application/atom+xml;charset=utf-8
...

Technically we could leverage this to skip the v3 trials there (which it does not support), but let's not complicate things unnecessarily...

@cdrnet

This comment has been minimized.

Show comment
Hide comment
@cdrnet

cdrnet Nov 10, 2016

Member

So, can we give it a spin? :)

Member

cdrnet commented Nov 10, 2016

So, can we give it a spin? :)

@forki forki merged commit 0611633 into fsprojects:master Nov 10, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 10, 2016

Member

yes please try it out

Member

forki commented Nov 10, 2016

yes please try it out

@cdrnet

This comment has been minimized.

Show comment
Hide comment
@cdrnet

cdrnet Nov 10, 2016

Member

Thanks, v3.27 works well for me. Suddenly commands like outdated give sensible output also on internal feeds :).

I don't have any other V2-only third-party feeds I can test against to verify myself unfortunately.

Member

cdrnet commented Nov 10, 2016

Thanks, v3.27 works well for me. Suddenly commands like outdated give sensible output also on internal feeds :).

I don't have any other V2-only third-party feeds I can test against to verify myself unfortunately.

@Vilmir

This comment has been minimized.

Show comment
Hide comment
@Vilmir

Vilmir Nov 18, 2016

@forki @cdrnet how shall we fix server-specific issues with OData?

Optional source brand config

We could allow users to optionally specify their source brand in the paket.dependencies:
source artifactory https://artifacts.sonova.com/artifactory/api/nuget/chinook-production

Source brand probing

Before querying the feed, a dummy query is sent in order to catch up a trace of the source brand in the response.

Then, when the source brand has been identified, specific fixes can be used at the right place.

Option 1 sounds better to me as we do not add yet another HTTP get before querying the feeds. It is also potentially a less buggy behavior. Users that do not actively specify their source brand will not be impacted.

Vilmir commented Nov 18, 2016

@forki @cdrnet how shall we fix server-specific issues with OData?

Optional source brand config

We could allow users to optionally specify their source brand in the paket.dependencies:
source artifactory https://artifacts.sonova.com/artifactory/api/nuget/chinook-production

Source brand probing

Before querying the feed, a dummy query is sent in order to catch up a trace of the source brand in the response.

Then, when the source brand has been identified, specific fixes can be used at the right place.

Option 1 sounds better to me as we do not add yet another HTTP get before querying the feeds. It is also potentially a less buggy behavior. Users that do not actively specify their source brand will not be impacted.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 18, 2016

Member

tbh I'm fine with URL guessing.

Member

forki commented Nov 18, 2016

tbh I'm fine with URL guessing.

@Vilmir

This comment has been minimized.

Show comment
Hide comment
@Vilmir

Vilmir Nov 18, 2016

You mean trying to determine the brand by parsing the URL string specified as source?

Vilmir commented Nov 18, 2016

You mean trying to determine the brand by parsing the URL string specified as source?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 18, 2016

Member

yes. we do that already for nuget.org and myget and things like that

Member

forki commented Nov 18, 2016

yes. we do that already for nuget.org and myget and things like that

@Vilmir

This comment has been minimized.

Show comment
Hide comment
@Vilmir

Vilmir Nov 18, 2016

That would work fine for public servers where the domain name reflects for sure the server type, but for Intranet private servers it is not true at 100%
Our company chose to create service names by their functions, decorrelated from their brand:

  • artifacts.sonova.com: today is actually Artifactory, may migrate to a less buggy product
  • symbols.sonova.com: ProGet and it will probably stay ProGet for a while. Because it just works for debug

Vilmir commented Nov 18, 2016

That would work fine for public servers where the domain name reflects for sure the server type, but for Intranet private servers it is not true at 100%
Our company chose to create service names by their functions, decorrelated from their brand:

  • artifacts.sonova.com: today is actually Artifactory, may migrate to a less buggy product
  • symbols.sonova.com: ProGet and it will probably stay ProGet for a while. Because it just works for debug

cdrnet added a commit to cdrnet/Paket that referenced this pull request Dec 9, 2016

NuGetV2-OData: retrieve versions in descending order for artifactory
Related to #2008 which was reverted in #2018 because it affected non-artifactory
servers negatively (in an unknonwn way).

This change adds an alternative variant of tryGetAllVersions to fetch the
versions in descending order (by publish date) instead of modifying the
original one.

GetVersions chooses to use the alternative variant in case the source uri
contains the string "artifactory". This is a very unreliable way of
discovering a server to be an artifactory one, but is in line with the
existing rules and should prevent affecting non-artifactory servers.

Ideally the decision should be more controllable than url matching, e.g.
by introducing the concept of source options.

@matthid matthid added this to NuGet API & Performance in Breaking Changes Aug 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment