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 <info@b-brahmer.de>

Co-authored-by: Sean Molenaar <SMillerDev@users.noreply.github.com>

9 files changed, 112 insertions, 165 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
-Subproject 397c735212bf1a06cfdd0cb7806c5a6ea79582b
+Subproject ffe84ea5dd43b568851549b3e241db150c12929