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>
2025-04-25 20:15:47 +02:00 · 2022-08-08 19:23:42 +02:00 · 2022-08-08 19:23:42 +02:00 · d4450ebad2
commit d4450ebad2
parent 496084d447
9 changed files with 110 additions and 163 deletions
--- 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
--- 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');
        }
--- 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')));
--- 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" {
--- 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 ]
+  run "./occ" news:feed:add "$user" "$NC_FEED" --title "Something-${BATS_SUITE_TEST_NUMBER}"
+  assert_success

-  if ! echo "$output" | grep '"ID":'; then
-    ret_status=$?
-    echo "Feed ID not returned"
-    return $ret_status
-  fi
+  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 ]
+  run ./occ news:feed:add "$user" "$NO_GUID_FEED" --title "Something-${BATS_SUITE_TEST_NUMBER}"
+  assert_failure

-  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
+  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 ]
+  assert_success

-  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:delete "$user" "$ID"
-  [ "$status" -eq 0 ]
+  assert_success
 }
--- 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" {
--- 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 ]
+  assert_success

-  if ! echo "$output" | grep '"author": "heise online",'; then
-    ret_status=$?
-    echo "Author fallback did not work"
-    return $ret_status
-  fi
-}
+
+  assert_output --partial '"author": "heise online",'
+}
--- 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"
--- a/tests/test_helper/bats-assert
+++ b/tests/test_helper/bats-assert
@ -1 +1 @@
-Subproject commit 397c735212bf1a06cfdd0cb7806c5a6ea79582bf
+Subproject commit ffe84ea5dd43b568851549b3e241db150c12929c