From d4450ebad2385058467580620272fc020f7dc3db Mon Sep 17 00:00:00 2001 From: Benjamin Brahmer Date: Mon, 8 Aug 2022 19:23:42 +0200 Subject: Change Autodiscover behaviour (#1860) * change autodiscover behaviour to only run if the provided url is not already a feed * Execute feed check after the final url is found Signed-off-by: Benjamin Brahmer Co-authored-by: Sean Molenaar --- CHANGELOG.md | 2 +- lib/Service/FeedServiceV2.php | 29 ++++++---- tests/Unit/Service/FeedServiceTest.php | 39 ++++++------- tests/api/feeds.bats | 3 +- tests/command/feeds.bats | 103 +++++++++++---------------------- tests/command/folders.bats | 8 +-- tests/command/items.bats | 85 ++++++++++----------------- tests/command/opml.bats | 8 +-- tests/test_helper/bats-assert | 2 +- 9 files changed, 113 insertions(+), 166 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d95a84d9..fbc2aaeac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ The format is mostly based on [Keep a Changelog](https://keepachangelog.com/en/1 # Unreleased ## [18.x.x] ### Changed - +- Change autodiscover to only run after fetching the given url has failed (#1860) ### Fixed # Releases diff --git a/lib/Service/FeedServiceV2.php b/lib/Service/FeedServiceV2.php index bb1be8dcb..3e02b3971 100644 --- a/lib/Service/FeedServiceV2.php +++ b/lib/Service/FeedServiceV2.php @@ -192,17 +192,6 @@ class FeedServiceV2 extends Service ?string $password = null, bool $full_discover = true ): Entity { - if ($this->existsForUser($userId, $feedUrl)) { - throw new ServiceConflictException('Feed with this URL exists'); - } - - if ($full_discover) { - $feeds = $this->explorer->discover($feedUrl); - if ($feeds !== []) { - $feedUrl = array_shift($feeds); - } - } - try { /** * @var Feed $feed @@ -211,9 +200,25 @@ class FeedServiceV2 extends Service list($feed, $items) = $this->feedFetcher->fetch($feedUrl, $full_text, $user, $password); } catch (ReadErrorException $ex) { $this->logger->debug($ex->getMessage()); - throw new ServiceNotFoundException($ex->getMessage()); + if ($full_discover === false) { + throw new ServiceNotFoundException($ex->getMessage()); + } + $this->logger->warning("No valid feed found at URL, attempting auto discovery"); + $feeds = $this->explorer->discover($feedUrl); + if ($feeds !== []) { + $feedUrl = array_shift($feeds); + } + try { + list($feed, $items) = $this->feedFetcher->fetch($feedUrl, $full_text, $user, $password); + } catch (ReadErrorException $ex) { + throw new ServiceNotFoundException($ex->getMessage()); + } } + if ($this->existsForUser($userId, $feedUrl)) { + throw new ServiceConflictException('Feed with this URL exists'); + } + if ($feed === null) { throw new ServiceNotFoundException('Failed to fetch feed'); } diff --git a/tests/Unit/Service/FeedServiceTest.php b/tests/Unit/Service/FeedServiceTest.php index 9f4743777..f0c367b7a 100644 --- a/tests/Unit/Service/FeedServiceTest.php +++ b/tests/Unit/Service/FeedServiceTest.php @@ -165,21 +165,14 @@ class FeedServiceTest extends TestCase public function testCreateDoesNotFindFeed() { - $ex = new ReadErrorException('hi'); $url = 'test'; - $this->mapper->expects($this->once()) - ->method('findByURL') - ->with($this->uid, $url) - ->willReturn(new Feed()); - - $this->fetcher->expects($this->never()) + $this->fetcher->expects($this->exactly(2)) ->method('fetch') ->with($url) - ->will($this->throwException($ex)); + ->will($this->throwException(new ReadErrorException('There is no feed'))); - $this->expectException(ServiceConflictException::class); - $this->expectExceptionMessage('Feed with this URL exists'); + $this->expectException(ServiceNotFoundException::class); $this->class->create($this->uid, $url, 1); } @@ -209,15 +202,16 @@ class FeedServiceTest extends TestCase ->method('findByURL') ->with($this->uid, $url) ->will($this->throwException(new DoesNotExistException('no'))); - $this->explorer->expects($this->once()) - ->method('discover') - ->with($url) - ->will($this->returnValue([])); + $this->explorer->expects($this->never()) + ->method('discover') + ->with($url) + ->will($this->returnValue([])); $this->fetcher->expects($this->once()) ->method('fetch') ->with($url) ->will($this->returnValue($return)); + $this->mapper->expects($this->once()) ->method('insert') ->with($createdFeed) @@ -268,7 +262,7 @@ class FeedServiceTest extends TestCase ->method('findByURL') ->with($this->uid, $url) ->will($this->throwException(new DoesNotExistException('no'))); - $this->explorer->expects($this->once()) + $this->explorer->expects($this->never()) ->method('discover') ->with($url) ->will($this->returnValue([])); @@ -326,16 +320,19 @@ class FeedServiceTest extends TestCase $this->mapper->expects($this->once()) ->method('findByURL') - ->with($this->uid, $url) + ->with($this->uid, 'http://discover.test') ->will($this->throwException(new DoesNotExistException('no'))); $this->explorer->expects($this->once()) ->method('discover') ->with($url) ->will($this->returnValue(['http://discover.test'])); - $this->fetcher->expects($this->once()) + $this->fetcher->expects($this->exactly(2)) ->method('fetch') - ->with('http://discover.test') - ->will($this->returnValue($return)); + ->withConsecutive( + ['http://test'], + ['http://discover.test'] + ) + ->willReturnOnConsecutiveCalls($this->throwException(new ReadErrorException('There is no feed')), $this->returnValue($return)); $this->mapper->expects($this->once()) ->method('insert') @@ -434,12 +431,12 @@ class FeedServiceTest extends TestCase $url = 'http://test'; $folderId = 10; - $this->fetcher->expects($this->once()) + $this->fetcher->expects($this->exactly(2)) ->method('fetch') ->with($url) ->will($this->throwException(new ReadErrorException('ERROR'))); - $this->mapper->expects($this->once()) + $this->mapper->expects($this->never()) ->method('findByURL') ->with($this->uid, $url) ->will($this->throwException(new DoesNotExistException('no'))); diff --git a/tests/api/feeds.bats b/tests/api/feeds.bats index 054fd776a..d789a554f 100644 --- a/tests/api/feeds.bats +++ b/tests/api/feeds.bats @@ -37,8 +37,7 @@ teardown() { # run is not working here. output=$(http --ignore-stdin -b -a ${user}:${user} POST ${BASE_URLv1}/feeds url=$NC_FEED | jq '.feeds | .[0].url') - # self reference of feed is used here - assert_output '"https://nextcloud.com/feed/"' + assert_output '"https://nextcloud.com/blog/static-feed/"' } @test "[$TESTSUITE] Create new inside folder" { diff --git a/tests/command/feeds.bats b/tests/command/feeds.bats index 5ad52dc30..97b4c27d1 100644 --- a/tests/command/feeds.bats +++ b/tests/command/feeds.bats @@ -1,50 +1,41 @@ #!/usr/bin/env bats -load "helpers/settings" +setup(){ + load "../test_helper/bats-support/load" + load "../test_helper/bats-assert/load" + load "helpers/settings" +} TESTSUITE="Feeds" teardown() { - ID=$(./occ news:feed:list 'admin' | grep "Something-${BATS_SUITE_TEST_NUMBER}" -2 | head -1 | grep -oE '[0-9]*') - if [ -n "$ID" ]; then - ./occ news:feed:delete "$user" "$ID" - fi + ID_LIST=($(./occ news:feed:list 'admin' | grep -Po '"id": \K([0-9]+)' | tr '\n' ' ')) + for ID in $ID_LIST; do + ./occ news:feed:delete "$user" "$ID" -v + done } @test "[$TESTSUITE] Create new" { - run "./occ" news:feed:add "$user" "$NC_FEED" - [ "$status" -eq 0 ] - - if ! echo "$output" | grep '"ID":'; then - ret_status=$? - echo "Feed ID not returned" - return $ret_status - fi + run "./occ" news:feed:add "$user" "$NC_FEED" --title "Something-${BATS_SUITE_TEST_NUMBER}" + assert_success + + assert_output --partial "Something-${BATS_SUITE_TEST_NUMBER}" } @test "[$TESTSUITE] Add feed without GUIDs" { - run ./occ news:feed:add "$user" "$NO_GUID_FEED" - [ "$status" -ne 0 ] - - if ! echo "$output" | grep "No parser can handle this stream"; then - ret_status=$? - echo "Malformed feed exception wasn't properly caught" - return $ret_status - fi + run ./occ news:feed:add "$user" "$NO_GUID_FEED" --title "Something-${BATS_SUITE_TEST_NUMBER}" + assert_failure + + assert_output "Malformed feed: item has no GUID" } @test "[$TESTSUITE] List all" { ./occ news:feed:add "$user" "$NC_FEED" --title "Something-${BATS_SUITE_TEST_NUMBER}" run ./occ news:feed:list "$user" - [ "$status" -eq 0 ] - - if ! echo "$output" | grep "Something-${BATS_SUITE_TEST_NUMBER}"; then - ret_status=$? - echo "Feed not found in list" - return $ret_status - fi + assert_success + assert_output --partial "Something-${BATS_SUITE_TEST_NUMBER}" } @test "[$TESTSUITE] Favicon" { @@ -52,65 +43,41 @@ teardown() { ./occ news:feed:add "$user" "$HEISE_FEED" --title "Something-${BATS_SUITE_TEST_NUMBER}" run ./occ news:feed:list "$user" - [ "$status" -eq 0 ] + assert_success - if ! echo "$output" | grep -F '"faviconLink": "https:\/\/nextcloud.com\/media\/screenshot-150x150.png"'; then - ret_status=$? - echo "Logo test failed" - return $ret_status - fi - - if ! echo "$output" | grep -F '"faviconLink": "https:\/\/www.heise.de\/favicon.ico"'; then - ret_status=$? - echo "Favicon test failed" - return $ret_status - fi + assert_output --partial '"faviconLink": "https:\/\/nextcloud.com\/wp-content\/uploads\/2022\/03\/favicon.png",' + assert_output --partial '"faviconLink": "https:\/\/www.heise.de\/favicon.ico?v=JykvN0w9Ye",' } @test "[$TESTSUITE] List all items" { - ./occ news:feed:add "$user" "https://github.com/nextcloud/news/releases.atom" --title "Something-${BATS_SUITE_TEST_NUMBER}" + ID=$(./occ news:feed:add "$user" "https://github.com/nextcloud/news/releases.atom" --title "Something-${BATS_SUITE_TEST_NUMBER}" | grep -Po '"id": \K([0-9]+)') TAG=$(curl --silent "https://api.github.com/repos/nextcloud/news/releases/latest" | grep '"tag_name":' | sed -E 's/.*"([^"]+)".*/\1/') - ID=$(./occ news:feed:list 'admin' | grep 'github\.com' -1 | head -1 | grep -oE '[0-9]*') run ./occ news:item:list-feed "$user" "$ID" --limit 200 - [ "$status" -eq 0 ] + assert_success - if ! echo "$output" | grep "$TAG"; then - ret_status=$? - echo "Release not found in list" - return $ret_status - fi + assert_output --partial $TAG } @test "[$TESTSUITE] Read all" { - ./occ news:feed:add "$user" "$NC_FEED" --title "Something-${BATS_SUITE_TEST_NUMBER}" + ID=$(./occ news:feed:add "$user" "$NC_FEED" --title "Something-${BATS_SUITE_TEST_NUMBER}" | grep -Po '"id": \K([0-9]+)') - run ./occ news:feed:list "$user" - [ "$status" -eq 0 ] - - echo "$output" | grep "Something-${BATS_SUITE_TEST_NUMBER}" - - ID=$(./occ news:feed:list 'admin' | grep "Something-${BATS_SUITE_TEST_NUMBER}" -2 | head -1 | grep -oE '[0-9]*') run ./occ news:feed:read "$user" "$ID" -v - [ "$status" -eq 0 ] - if ! echo "$output" | grep "items as read"; then - ret_status=$? - echo "Feed not read" - return $ret_status - fi + assert_output --partial "items as read" + + # Needed for some reason because the teardown doesn't work after this step. + run ./occ news:feed:delete "$user" "$ID" -v + assert_success } -@test "[$TESTSUITE] Delete all" { - ./occ news:feed:add "$user" "$NC_FEED" --title "Something-${BATS_SUITE_TEST_NUMBER}" +@test "[$TESTSUITE] Delete one" { + ID=$(./occ news:feed:add "$user" "$NC_FEED" --title "Something-${BATS_SUITE_TEST_NUMBER}" | grep -Po '"id": \K([0-9]+)') run ./occ news:feed:list "$user" - [ "$status" -eq 0 ] - - echo "$output" | grep "Something-${BATS_SUITE_TEST_NUMBER}" + assert_success - ID=$(./occ news:feed:list 'admin' | grep "Something-${BATS_SUITE_TEST_NUMBER}" -2 | head -1 | grep -oE '[0-9]*') run ./occ news:feed:delete "$user" "$ID" - [ "$status" -eq 0 ] + assert_success } diff --git a/tests/command/folders.bats b/tests/command/folders.bats index 48b15f05b..2568c13c9 100644 --- a/tests/command/folders.bats +++ b/tests/command/folders.bats @@ -5,10 +5,10 @@ load "helpers/settings" TESTSUITE="Folders" teardown() { - ID=$(./occ news:folder:list 'admin' | grep "Something-${BATS_SUITE_TEST_NUMBER}" -1 | head -1 | grep -oE '[0-9]*') - if [ -n "$ID" ]; then - ./occ news:folder:delete "$user" "$ID" - fi + ID_LIST=($(./occ news:feed:list 'admin' | grep -Po '"id": \K([0-9]+)' | tr '\n' ' ')) + for ID in $ID_LIST; do + ./occ news:feed:delete "$user" "$ID" + done } @test "[$TESTSUITE] Create new" { diff --git a/tests/command/items.bats b/tests/command/items.bats index 8a7111299..ab2fb312c 100644 --- a/tests/command/items.bats +++ b/tests/command/items.bats @@ -1,98 +1,77 @@ #!/usr/bin/env bats -load "helpers/settings" TESTSUITE="Items" setup() { - ./occ news:feed:add "$user" "https://github.com/nextcloud/news/releases.atom" --title "Something-${BATS_SUITE_TEST_NUMBER}" - + load "../test_helper/bats-support/load" + load "../test_helper/bats-assert/load" + load "helpers/settings" TAG=$(curl --silent "https://api.github.com/repos/nextcloud/news/releases/latest" | grep '"tag_name":' | sed -E 's/.*"([^"]+)".*/\1/') - ID=$(./occ news:feed:list 'admin' | grep 'github\.com' -1 | head -1 | grep -oE '[0-9]*') + } -teardown() { - if [ -n "$ID" ]; then +teardown(){ + ID_LIST=($(./occ news:feed:list 'admin' | grep -Po '"id": \K([0-9]+)' | tr '\n' ' ')) + for ID in $ID_LIST; do ./occ news:feed:delete "$user" "$ID" - fi + done } @test "[$TESTSUITE] List 200 items in feed" { + ID=$(./occ news:feed:add "$user" "https://github.com/nextcloud/news/releases.atom" --title "Something-${BATS_SUITE_TEST_NUMBER}" | grep -Po '"id": \K([0-9]+)') run ./occ news:item:list-feed "$user" "$ID" --limit 200 - [ "$status" -eq 0 ] + assert_success - if ! echo "$output" | grep "$TAG"; then - ret_status=$? - echo "Release not found in feed list" - return $ret_status - fi + assert_output --partial $TAG } @test "[$TESTSUITE] List all items in feed" { + ID=$(./occ news:feed:add "$user" "https://github.com/nextcloud/news/releases.atom" --title "Something-${BATS_SUITE_TEST_NUMBER}" | grep -Po '"id": \K([0-9]+)') run ./occ news:item:list-feed "$user" "$ID" --limit 0 - [ "$status" -eq 0 ] + assert_success - if ! echo "$output" | grep "$TAG"; then - ret_status=$? - echo "Release not found in feed list" - return $ret_status - fi + assert_output --partial $TAG } @test "[$TESTSUITE] List 200 items in folder" { + ID=$(./occ news:feed:add "$user" "https://github.com/nextcloud/news/releases.atom" --title "Something-${BATS_SUITE_TEST_NUMBER}" | grep -Po '"id": \K([0-9]+)') run ./occ news:item:list-folder "$user" --limit 200 - [ "$status" -eq 0 ] + assert_success - if ! echo "$output" | grep "$TAG"; then - ret_status=$? - echo "Release not found in folder list" - return $ret_status - fi + assert_output --partial $TAG } @test "[$TESTSUITE] List all items in folder" { + ID=$(./occ news:feed:add "$user" "https://github.com/nextcloud/news/releases.atom" --title "Something-${BATS_SUITE_TEST_NUMBER}" | grep -Po '"id": \K([0-9]+)') run ./occ news:item:list-folder "$user" --limit 0 - [ "$status" -eq 0 ] + assert_success - if ! echo "$output" | grep "$TAG"; then - ret_status=$? - echo "Release not found in folder list" - return $ret_status - fi + assert_output --partial $TAG } @test "[$TESTSUITE] List 200 items" { + ID=$(./occ news:feed:add "$user" "https://github.com/nextcloud/news/releases.atom" --title "Something-${BATS_SUITE_TEST_NUMBER}" | grep -Po '"id": \K([0-9]+)') run ./occ news:item:list "$user" --limit 200 - [ "$status" -eq 0 ] + assert_success - if ! echo "$output" | grep "$TAG"; then - ret_status=$? - echo "Release not found in list" - return $ret_status - fi + assert_output --partial $TAG } @test "[$TESTSUITE] List all items" { + ID=$(./occ news:feed:add "$user" "https://github.com/nextcloud/news/releases.atom" --title "Something-${BATS_SUITE_TEST_NUMBER}" | grep -Po '"id": \K([0-9]+)') run ./occ news:item:list "$user" --limit 0 - [ "$status" -eq 0 ] + assert_success - if ! echo "$output" | grep "$TAG"; then - ret_status=$? - echo "Release not found in list" - return $ret_status - fi + assert_output --partial $TAG } @test "[$TESTSUITE] Test author fallback" { - ./occ news:feed:add "$user" $HEISE_FEED --title "Something-${BATS_SUITE_TEST_NUMBER}" - ID=$(./occ news:feed:list 'admin' | grep 'heise\.de' -1 | head -1 | grep -oE '[0-9]*') + ID=$(./occ news:feed:add "$user" $HEISE_FEED --title "Something-${BATS_SUITE_TEST_NUMBER}" | grep -Po '"id": \K([0-9]+)') run ./occ news:item:list-feed "$user" "$ID" --limit 200 - [ "$status" -eq 0 ] - - if ! echo "$output" | grep '"author": "heise online",'; then - ret_status=$? - echo "Author fallback did not work" - return $ret_status - fi -} \ No newline at end of file + assert_success + + + assert_output --partial '"author": "heise online",' +} diff --git a/tests/command/opml.bats b/tests/command/opml.bats index b2d576e8e..bfdc8bf93 100644 --- a/tests/command/opml.bats +++ b/tests/command/opml.bats @@ -5,14 +5,14 @@ load "helpers/settings" TESTSUITE="OPML" teardown() { - ID=$(./occ news:feed:list 'admin' | grep "Something-${BATS_SUITE_TEST_NUMBER}" -1 | head -1 | grep -oE '[0-9]*') - if [ -n "$ID" ]; then + ID_LIST=($(./occ news:feed:list 'admin' | grep -Po '"id": \K([0-9]+)' | tr '\n' ' ')) + for ID in $ID_LIST; do ./occ news:feed:delete "$user" "$ID" - fi + done } @test "[$TESTSUITE] Export" { - run ./occ news:feed:add "$user" "https://nextcloud.com/blog/static-feed/" + run ./occ news:feed:add "$user" "https://nextcloud.com/blog/static-feed/" --title "Something-${BATS_SUITE_TEST_NUMBER}" [ "$status" -eq 0 ] run ./occ news:opml:export "$user" diff --git a/tests/test_helper/bats-assert b/tests/test_helper/bats-assert index 397c73521..ffe84ea5d 160000 --- a/tests/test_helper/bats-assert +++ b/tests/test_helper/bats-assert @@ -1 +1 @@ -Subproject commit 397c735212bf1a06cfdd0cb7806c5a6ea79582bf +Subproject commit ffe84ea5dd43b568851549b3e241db150c12929c -- cgit v1.2.3