Include assets directly in GHRelease #977

skaldarnar · 2020-11-11T18:29:07Z

Description

Resolves #528

Instead of doing a separate request to fetch the assets associated with
a release this keeps a local list of the assets that are part of the
list releases endpoint.

See https://docs.github.com/en/free-pro-team@latest/rest/reference/repos#list-releases

The first commit c74fbbe expectedly fails at

[ERROR] Failures:
[ERROR]   LifecycleTest.testCreateRepository:39->uploadAsset:60->Assert.assertEquals:633->Assert.assertEquals:647->Assert.failNotEquals:835->Assert.fail:89 expected:<1> but was:<0>

as the call to release.getAssets() not returns the cached content instead of doing another request.

github-api/src/test/java/org/kohsuke/github/LifecycleTest.java

Lines 56 to 64 in 3a11b7c

    
           private GHAsset uploadAsset(GHRelease release) throws IOException { 
        
               GHAsset asset = release.uploadAsset(new File("LICENSE.txt"), "application/text"); 
        
               assertNotNull(asset); 
        
               List<GHAsset> assets = release.getAssets(); 
        
               assertEquals(1, assets.size()); 
        
               assertEquals("LICENSE.txt", assets.get(0).getName()); 
        
               return asset; 
        
           }

Therefore, the second commit d881bf6 brings back the previous functionality as fetchAssets(). I don't know a better name for this, and I also don't know whether this should be an option or not.

The life cycle tests use the feature that retrieving the assets from a release actually does a roundtrip to Github and sends another request. This indicates that re-purposing getAssets() to be the cached access might cause problems on consumers that potentially rely on this assumption, too.

Resolves hub4j#528 Instead of doing a separate request to fetch the assets associated with a release this keeps a local list of the assets that are part of the list releases endpoint. See https://docs.github.com/en/free-pro-team@latest/rest/reference/repos#list-releases

I don't know a better name for this, and I also don't know whether this should be an option or not. The life cycle tests use the feature that retrieving the assets from a release actually does a roundtrip to Github and sends another request. This indicates that re-purposing `getAssets()` to be the cached access might cause problems on consumers that potentially rely on this assumption, too.

skaldarnar · 2020-11-14T10:11:51Z

@bitwiseman pinging you because I cannot request you as reviewer 😉

@johnoliver is this what you had in mind when opening #528 ?

bitwiseman

This is a tough problem.

I think this change results in more correct behavior, but it also a breaking change in terms of the behavior of the existing method.

Someone expecting getAssets() to always return the up-to-date list of assets is going to get a very unpleasant surprise when this change goes in.

I would suggest you create a new method instead of modifying the old one (at least for now). Maybe call it getCachedAssets() and make it @Deprecated @Preview with comments explaining what is going on. Also make getAssets() @Deprecated (but not preview) with an explanation this the method is still usable but that the behavior will change in a future version.

That would unlock the behavior you want but not break existing users.

@Deprecation

Add '@Deprecation' and '@Preview' annotations to make the changes backwards compatible and prepare for a transition to the cached behavior as default. This explicitly leaves the variant for re-fetching assets under a different name.

skaldarnar · 2020-11-17T16:20:59Z

src/main/java/org/kohsuke/github/GHRelease.java

+     * @throws IOException
+     *             the io exception
+     */
+    public List<GHAsset> fetchAssets() throws IOException {


I left this in as I think this is a quite handy featuer to have (easily run queries to be up to date, if required). If you would rather see this removed please let me know. In that case, the test cases should also be revisitied to cover the same use case with just the new, cached behavior.

This seems reasonable. But since we're fetching, let's return a PagedIterable for more flexibility. This also means it makes sense to use the list* verb like we do on all other methods that return PagedIterable.

Suggested change

public List<GHAsset> fetchAssets() throws IOException {

public PagedIterable<GHAsset> listAssets() throws IOException {

bitwiseman

This is looking great. Some additional discussion and changes. After this we should be good to go.

Thanks!

bitwiseman · 2020-11-17T18:35:24Z

src/test/java/org/kohsuke/github/LifecycleTest.java

    }

    private void deleteAsset(GHRelease release, GHAsset asset) throws IOException {
        asset.delete();
-        assertEquals(0, release.getAssets().size());
+        assertEquals(0, release.getCachedAssets().size());


This seems strange. If the asset was cached in the release before deletion, it should be there after was well - that cache shouldn't change. Perhaps add an assert that the asset if present in getCachedAssets() before deletion?

The confusing part here might be that we're still dealing with the very first GHRelease from this test - even before we actually created an asset for it.

I'll take another look at the tests and see if I can add something in addition to make this clearer

Ah, I get it now.

I tried to figure out how this whole wiremock setup works - and gave up 😕 To be able to add an assertion like the one you mentioned I'd need to re-fetch the intermediate state of the release, e.g.,

// test that asset upload works GHAsset asset = uploadAsset(release); // get new "snapshot" of the release with cached asset GHRelease releaseWithAsset = repository.getReleases().get(0); // update the asset, check before and after with the new snapshot updateAsset(releaseWithAsset, asset); // repeat same pattern for 'delete'

However, it looks like the wiremocks do not account for additional calls to repository.getReleases().get(0) I suppose, at least they obviously don't return the new state of the release...

If I should change something in these tests I'd appreciate your help... 🙄

I need to make it easier to re-record tests, but in the meanwhile here's the instructions:
https://github.com/hub4j/github-api/blob/master/CONTRIBUTING.md#generating-a-new-snapshot

The instructions could be better too. 😢

But it is okay to leave this as it is.

bitwiseman · 2020-11-17T18:49:13Z

src/main/java/org/kohsuke/github/GHRelease.java

+     * @throws IOException
+     *             the io exception
+     */
+    public List<GHAsset> fetchAssets() throws IOException {


This seems reasonable. But since we're fetching, let's return a PagedIterable for more flexibility. This also means it makes sense to use the list* verb like we do on all other methods that return PagedIterable.

Suggested change

public List<GHAsset> fetchAssets() throws IOException {

public PagedIterable<GHAsset> listAssets() throws IOException {

bitwiseman · 2020-11-17T18:57:25Z

src/main/java/org/kohsuke/github/GHRelease.java

+     */
+    @Deprecated
+    @Preview
+    public List<GHAsset> getCachedAssets() {


API design question for you: Thinking about it a bit more, what do you think about using a bare property names for field access? I don't think this needs changing for this change, but I'm thinking about what this should look like in v2.

public List<GHAsset> assets() {

I would really like this kind of simplified naming 👍 🤓

bitwiseman · 2020-11-17T18:58:40Z

src/main/java/org/kohsuke/github/GHRelease.java

    public List<GHAsset> getAssets() throws IOException {
-        Requester builder = owner.root.createRequest();
+        return fetchAssets();


Given we change the name and return type of fetchAssets():

Suggested change

return fetchAssets();

return listAssets().toList();

bitwiseman · 2020-11-17T19:00:35Z

src/test/java/org/kohsuke/github/LifecycleTest.java

@@ -45,18 +45,20 @@ public void testCreateRepository() throws IOException {

    private void updateAsset(GHRelease release, GHAsset asset) throws IOException {
        asset.setLabel("test label");
-        assertEquals("test label", release.getAssets().get(0).getLabel());
+        assertEquals("test label", release.fetchAssets().get(0).getLabel());


Keep the test pointing to the deprecated method since it wraps the new method and we want to show the behavior hasn't changed.

Suggested change

assertEquals("test label", release.fetchAssets().get(0).getLabel());

assertEquals("test label", release.getAssets().get(0).getLabel());

skaldarnar added 2 commits November 11, 2020 19:16

skaldarnar mentioned this pull request Nov 11, 2020

[WIP] Epic: refactor package management MovingBlocks/TerasologyLauncher#609

Closed

skaldarnar mentioned this pull request Nov 15, 2020

feat: add Github repository adapter to fetch releases from Github MovingBlocks/TerasologyLauncher#616

Merged

bitwiseman requested changes Nov 17, 2020

View reviewed changes

Make changes backwards compatible

98b0679

Add '@Deprecation' and '@Preview' annotations to make the changes backwards compatible and prepare for a transition to the cached behavior as default. This explicitly leaves the variant for re-fetching assets under a different name.

skaldarnar commented Nov 17, 2020

View reviewed changes

skaldarnar requested a review from bitwiseman November 17, 2020 16:21

bitwiseman requested changes Nov 17, 2020

View reviewed changes

skaldarnar added 4 commits November 18, 2020 19:27

rename 'fetchAssets' to 'listAssets' and return PagedIterable

2676ef2

rename 'getCachedAssets' to 'assets' (bare property name as getter))

68a82be

revert unwanted assertion change

5d796d1

fix formatting

79c7dd9

skaldarnar requested a review from bitwiseman November 18, 2020 20:32

bitwiseman approved these changes Nov 18, 2020

View reviewed changes

bitwiseman merged commit 78f533b into hub4j:master Nov 18, 2020

skaldarnar deleted the feat/528-ghrelease-assets branch November 22, 2023 19:28

skaldarnar mentioned this pull request Nov 22, 2023

Pre-fetched assets on GHReleases #1750

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include assets directly in GHRelease #977

Include assets directly in GHRelease #977

skaldarnar commented Nov 11, 2020 •

edited

skaldarnar commented Nov 14, 2020

bitwiseman left a comment

skaldarnar Nov 17, 2020

bitwiseman Nov 17, 2020

bitwiseman left a comment

bitwiseman Nov 17, 2020

skaldarnar Nov 17, 2020

bitwiseman Nov 17, 2020

skaldarnar Nov 18, 2020

bitwiseman Nov 18, 2020

bitwiseman Nov 18, 2020

bitwiseman Nov 17, 2020

bitwiseman Nov 17, 2020

skaldarnar Nov 17, 2020

bitwiseman Nov 17, 2020

bitwiseman Nov 17, 2020

	private GHAsset uploadAsset(GHRelease release) throws IOException {
	GHAsset asset = release.uploadAsset(new File("LICENSE.txt"), "application/text");
	assertNotNull(asset);
	List<GHAsset> assets = release.getAssets();
	assertEquals(1, assets.size());
	assertEquals("LICENSE.txt", assets.get(0).getName());

	return asset;
	}

	public List<GHAsset> fetchAssets() throws IOException {
	public PagedIterable<GHAsset> listAssets() throws IOException {

	assertEquals("test label", release.fetchAssets().get(0).getLabel());
	assertEquals("test label", release.getAssets().get(0).getLabel());

Include assets directly in GHRelease #977

Include assets directly in GHRelease #977

Conversation

skaldarnar commented Nov 11, 2020 • edited

Description

skaldarnar commented Nov 14, 2020

bitwiseman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitwiseman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skaldarnar commented Nov 11, 2020 •

edited