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

Pre-fetched assets on GHReleases #1750

Open
skaldarnar opened this issue Nov 22, 2023 · 3 comments
Open

Pre-fetched assets on GHReleases #1750

skaldarnar opened this issue Nov 22, 2023 · 3 comments

Comments

@skaldarnar
Copy link
Contributor

skaldarnar commented Nov 22, 2023

Hi,

I'm a bit confused about the situation on pre-fetched/cached assets on GHRelease. I'd like to avoid additional API calls, see also my previous PR #977.

Can we make getAssets() use the behavior of assets() by now, and remove assets()? Or how do you want to handle this?

assets() lost it's @Preview/@BetaApi annotation in 4e7ac70#diff-d6ca05bee054ef7153d926fbe1c6cf67524ecaf7298ada1d9fff838119dceb95L263, maybe by accident. I guess if it would have kept the annotation, it would by now a non-deprecated preview (see #1184).

/**
* Get the cached assets.
*
* @return the assets
*
* @deprecated This should be the default behavior of {@link #getAssets()} in a future release. This method is
* introduced in addition to enable a transition to using cached asset information while keeping the
* existing logic in place for backwards compatibility.
*/
@Deprecated
public List<GHAsset> assets() {
return Collections.unmodifiableList(assets);
}
/**
* Re-fetch the assets of this release.
*
* @return the assets
* @throws IOException
* the io exception
* @deprecated The behavior of this method will change in a future release. It will then provide cached assets as
* provided by {@link #assets()}. Use {@link #listAssets()} instead to fetch up-to-date information of
* assets.
*/
@Deprecated
public List<GHAsset> getAssets() throws IOException {
return listAssets().toList();
}

@bitwiseman
Copy link
Member

@skaldarnar
Have you tested this behavior with a release with a large number of assets? The documentation for Releases and Release Assets is not clear about any limitations on the number of assets returned with a release, similar the limit on the number of files returned with a commit (see #1679). I'm concerned that there is a limit, but that they've failed to document it.

Please try a test with a repo and at least 400 assets in a release and report back. If there really isn't a limit on the number of assets returned, great we can consolidate these calls to a simpler underlying implemenation. If there is a limit, we can still consolidate them, but it will need to be behavior similar to #1679, where we detect if when we're at or above a certain number of assets and switch to from cached to querying a new list.

@skaldarnar
Copy link
Contributor Author

@bitwiseman I did a quick test with https://github.com/adoptium/temurin17-binaries/releases/tag/jdk-17.0.8.1%2B1 which has 290 assets in the release (+2 auto-generated assets for the source code). When using the Github REST API via gh all 290 assets are listed in there. Not sure whether this does any pagination under the hood.

gh api \
  -H "Accept: application/vnd.github+json" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  /repos/adoptium/temurin17-binaries/releases/tags/jdk-17.0.8.1%2B1 > temurin-releases.json

jq '.assets | length' temurin-releases.json

I'll try to find repos with more release assets or create some dummy repo myself for futher testing.

Interestingly, when fetching the list of release assets via List release assets there's a page limit of 100 according to the documentation 🤔

@bitwiseman
Copy link
Member

bitwiseman commented Dec 3, 2023

@skaldarnar
No, pagination is not an under the hood thing, it takes extra calls.

The page size limit of 100 is standard across the rest API in GitHub.

290 is a reasonable test. If someone exceeds that, we can file a bug to fix it.

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

No branches or pull requests

2 participants