From 3e656efb0088b180665e224a68adde061e86786b Mon Sep 17 00:00:00 2001 From: Rashad Date: Mon, 21 Oct 2024 02:42:49 +0530 Subject: [PATCH 1/4] Added include func for search api --- app/Api/ApiEntityListFormatter.php | 42 ++++++++++- app/Search/SearchApiController.php | 69 ++++++++++++++--- dev/api/requests/search-all.http | 2 +- dev/api/responses/search-all.json | 7 +- tests/Api/SearchApiTest.php | 117 ++++++++++++++++++++++++++++- 5 files changed, 217 insertions(+), 20 deletions(-) diff --git a/app/Api/ApiEntityListFormatter.php b/app/Api/ApiEntityListFormatter.php index 436d66d59..23fa8e6ea 100644 --- a/app/Api/ApiEntityListFormatter.php +++ b/app/Api/ApiEntityListFormatter.php @@ -3,6 +3,7 @@ namespace BookStack\Api; use BookStack\Entities\Models\Entity; +use BookStack\Entities\Models\Page; class ApiEntityListFormatter { @@ -12,6 +13,11 @@ class ApiEntityListFormatter */ protected array $list = []; + /** + * Whether to include related titles in the response. + */ + protected bool $includeRelatedTitles = false; + /** * The fields to show in the formatted data. * Can be a plain string array item for a direct model field (If existing on model). @@ -20,8 +26,16 @@ class ApiEntityListFormatter * @var array */ protected array $fields = [ - 'id', 'name', 'slug', 'book_id', 'chapter_id', 'draft', - 'template', 'priority', 'created_at', 'updated_at', + 'id', + 'name', + 'slug', + 'book_id', + 'chapter_id', + 'draft', + 'template', + 'priority', + 'created_at', + 'updated_at', ]; public function __construct(array $list) @@ -62,6 +76,30 @@ class ApiEntityListFormatter return $this; } + /** + * Enable the inclusion of related book and chapter titles in the response. + */ + public function withRelatedTitles(): self + { + $this->includeRelatedTitles = true; + + $this->withField('book_title', function (Entity $entity) { + if (method_exists($entity, 'book')) { + return $entity->book?->name; + } + return null; + }); + + $this->withField('chapter_title', function (Entity $entity) { + if ($entity instanceof Page && $entity->chapter_id) { + return optional($entity->getAttribute('chapter'))->name; + } + return null; + }); + + return $this; + } + /** * Format the data and return an array of formatted content. * @return array[] diff --git a/app/Search/SearchApiController.php b/app/Search/SearchApiController.php index d1619e118..5072bd3b4 100644 --- a/app/Search/SearchApiController.php +++ b/app/Search/SearchApiController.php @@ -14,12 +14,23 @@ class SearchApiController extends ApiController protected $rules = [ 'all' => [ - 'query' => ['required'], - 'page' => ['integer', 'min:1'], - 'count' => ['integer', 'min:1', 'max:100'], + 'query' => ['required'], + 'page' => ['integer', 'min:1'], + 'count' => ['integer', 'min:1', 'max:100'], + 'include' => ['string', 'regex:/^[a-zA-Z,]*$/'], ], ]; + /** + * Valid include parameters and their corresponding formatter methods. + * These parameters allow for additional related data, like titles or tags, + * to be included in the search results when requested via the API. + */ + protected const VALID_INCLUDES = [ + 'titles' => 'withRelatedTitles', + 'tags' => 'withTags', + ]; + public function __construct(SearchRunner $searchRunner, SearchResultsFormatter $resultsFormatter) { $this->searchRunner = $searchRunner; @@ -33,6 +44,13 @@ class SearchApiController extends ApiController * for a full list of search term options. Results contain a 'type' property to distinguish * between: bookshelf, book, chapter & page. * + * This method now supports the 'include' parameter, which allows API clients to specify related + * fields (such as titles or tags) that should be included in the search results. + * + * The 'include' parameter is a comma-separated string. For example, adding `include=titles,tags` + * will include both titles and tags in the API response. If the parameter is not provided, only + * basic entity data will be returned. + * * The paging parameters and response format emulates a standard listing endpoint * but standard sorting and filtering cannot be done on this endpoint. If a count value * is provided this will only be taken as a suggestion. The results in the response @@ -45,22 +63,49 @@ class SearchApiController extends ApiController $options = SearchOptions::fromString($request->get('query') ?? ''); $page = intval($request->get('page', '0')) ?: 1; $count = min(intval($request->get('count', '0')) ?: 20, 100); + $includes = $this->parseIncludes($request->get('include', '')); $results = $this->searchRunner->searchEntities($options, 'all', $page, $count); $this->resultsFormatter->format($results['results']->all(), $options); - $data = (new ApiEntityListFormatter($results['results']->all())) - ->withType()->withTags() - ->withField('preview_html', function (Entity $entity) { - return [ - 'name' => (string) $entity->getAttribute('preview_name'), - 'content' => (string) $entity->getAttribute('preview_content'), - ]; - })->format(); + $formatter = new ApiEntityListFormatter($results['results']->all()); + $formatter->withType(); // Always include type as it's essential for search results + + foreach ($includes as $include) { + if (isset(self::VALID_INCLUDES[$include])) { + $method = self::VALID_INCLUDES[$include]; + $formatter->$method(); + } + } + + $formatter->withField('preview_html', function (Entity $entity) { + return [ + 'name' => (string) $entity->getAttribute('preview_name'), + 'content' => (string) $entity->getAttribute('preview_content'), + ]; + }); return response()->json([ - 'data' => $data, + 'data' => $formatter->format(), 'total' => $results['total'], ]); } + + /** + * Parse and validate the include parameter. + * + * @param string $includeString Comma-separated list of includes + * @return array + */ + protected function parseIncludes(string $includeString): array + { + if (empty($includeString)) { + return []; + } + + return array_filter( + explode(',', strtolower($includeString)), + fn($include) => isset (self::VALID_INCLUDES[$include]) + ); + } } diff --git a/dev/api/requests/search-all.http b/dev/api/requests/search-all.http index ee5223816..7fa1a304e 100644 --- a/dev/api/requests/search-all.http +++ b/dev/api/requests/search-all.http @@ -1 +1 @@ -GET /api/search?query=cats+{created_by:me}&page=1&count=2 \ No newline at end of file +GET /api/search?query=cats+{created_by:me}&page=1&count=2&include=titles,tags diff --git a/dev/api/responses/search-all.json b/dev/api/responses/search-all.json index 2c7584e3f..bb45b7959 100644 --- a/dev/api/responses/search-all.json +++ b/dev/api/responses/search-all.json @@ -9,6 +9,7 @@ "updated_at": "2021-11-14T15:57:35.000000Z", "type": "chapter", "url": "https://example.com/books/my-book/chapter/a-chapter-for-cats", + "book_title": "Cats", "preview_html": { "name": "A chapter for cats", "content": "...once a bunch of cats named tony...behaviour of cats is unsuitable" @@ -27,6 +28,8 @@ "updated_at": "2021-11-14T15:56:49.000000Z", "type": "page", "url": "https://example.com/books/my-book/page/the-hows-and-whys-of-cats", + "book_title": "Cats", + "chapter_title": "A chapter for cats", "preview_html": { "name": "The hows and whys of cats", "content": "...people ask why cats? but there are...the reason that cats are fast are due to..." @@ -56,6 +59,8 @@ "updated_at": "2021-11-14T16:02:39.000000Z", "type": "page", "url": "https://example.com/books/my-book/page/how-advanced-are-cats", + "book_title": "Cats", + "chapter_title": "A chapter for cats", "preview_html": { "name": "How advanced are cats?", "content": "cats are some of the most advanced animals in the world." @@ -64,4 +69,4 @@ } ], "total": 3 -} \ No newline at end of file +} diff --git a/tests/Api/SearchApiTest.php b/tests/Api/SearchApiTest.php index 2a186e8d6..b80ed4530 100644 --- a/tests/Api/SearchApiTest.php +++ b/tests/Api/SearchApiTest.php @@ -2,6 +2,7 @@ namespace Tests\Api; +use BookStack\Activity\Models\Tag; use BookStack\Entities\Models\Book; use BookStack\Entities\Models\Bookshelf; use BookStack\Entities\Models\Chapter; @@ -45,7 +46,7 @@ class SearchApiTest extends TestCase $resp = $this->actingAsApiAdmin()->getJson($this->baseEndpoint . '?query=superuniquevalue'); $resp->assertJsonFragment([ 'type' => 'page', - 'url' => $page->getUrl(), + 'url' => $page->getUrl(), ]); } @@ -57,10 +58,10 @@ class SearchApiTest extends TestCase $resp = $this->actingAsApiAdmin()->getJson($this->baseEndpoint . '?query=superuniquevalue'); $resp->assertJsonFragment([ - 'type' => 'book', - 'url' => $book->getUrl(), + 'type' => 'book', + 'url' => $book->getUrl(), 'preview_html' => [ - 'name' => 'name with superuniquevalue within', + 'name' => 'name with superuniquevalue within', 'content' => 'Description with superuniquevalue within', ], ]); @@ -74,4 +75,112 @@ class SearchApiTest extends TestCase $resp = $this->actingAsApiEditor()->get($this->baseEndpoint . '?query=myqueryvalue'); $resp->assertOk(); } + + public function test_all_endpoint_includes_book_and_chapter_titles_when_requested() + { + $this->actingAsApiEditor(); + + $book = $this->entities->book(); + $chapter = $this->entities->chapter(); + $page = $this->entities->newPage(); + + $book->name = 'My Test Book'; + $book->save(); + + $chapter->name = 'My Test Chapter'; + $chapter->book_id = $book->id; + $chapter->save(); + + $page->name = 'My Test Page With UniqueSearchTerm'; + $page->book_id = $book->id; + $page->chapter_id = $chapter->id; + $page->save(); + + $page->indexForSearch(); + + // Test without include parameter + $resp = $this->getJson($this->baseEndpoint . '?query=UniqueSearchTerm'); + $resp->assertOk(); + $resp->assertDontSee('book_title'); + $resp->assertDontSee('chapter_title'); + + // Test with include parameter + $resp = $this->getJson($this->baseEndpoint . '?query=UniqueSearchTerm&include=titles'); + $resp->assertOk(); + $resp->assertJsonFragment([ + 'name' => 'My Test Page With UniqueSearchTerm', + 'book_title' => 'My Test Book', + 'chapter_title' => 'My Test Chapter', + 'type' => 'page' + ]); + } + + public function test_all_endpoint_validates_include_parameter() + { + $this->actingAsApiEditor(); + + // Test invalid include value + $resp = $this->getJson($this->baseEndpoint . '?query=test&include=invalid'); + $resp->assertOk(); + $resp->assertDontSee('book_title'); + + // Test SQL injection attempt + $resp = $this->getJson($this->baseEndpoint . '?query=test&include=titles;DROP TABLE users'); + $resp->assertStatus(422); + + // Test multiple includes + $resp = $this->getJson($this->baseEndpoint . '?query=test&include=titles,tags'); + $resp->assertOk(); + } + + public function test_all_endpoint_includes_tags_when_requested() + { + $this->actingAsApiEditor(); + + // Create a page and give it a unique name for search + $page = $this->entities->page(); + $page->name = 'Page With UniqueSearchTerm'; + $page->save(); + + // Save tags to the page using the existing saveTagsToEntity method + $tags = [ + ['name' => 'SampleTag', 'value' => 'SampleValue'] + ]; + app(\BookStack\Activity\TagRepo::class)->saveTagsToEntity($page, $tags); + + // Ensure the page is indexed for search + $page->indexForSearch(); + + // Test without the "tags" include + $resp = $this->getJson($this->baseEndpoint . '?query=UniqueSearchTerm'); + $resp->assertOk(); + $resp->assertDontSee('tags'); + + // Test with the "tags" include + $resp = $this->getJson($this->baseEndpoint . '?query=UniqueSearchTerm&include=tags'); + $resp->assertOk(); + + // Assert that tags are included in the response + $resp->assertJsonFragment([ + 'name' => 'SampleTag', + 'value' => 'SampleValue', + ]); + + // Optionally: check the structure to match the tag order as well + $resp->assertJsonStructure([ + 'data' => [ + '*' => [ + 'tags' => [ + '*' => [ + 'name', + 'value', + 'order', + ], + ], + ], + ], + ]); + } + + } From 90a80705180d003a1e896bc307d3abc1720366dc Mon Sep 17 00:00:00 2001 From: Rashad Date: Mon, 21 Oct 2024 03:01:33 +0530 Subject: [PATCH 2/4] Eager loading for titles --- app/Api/ApiEntityListFormatter.php | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/app/Api/ApiEntityListFormatter.php b/app/Api/ApiEntityListFormatter.php index 23fa8e6ea..2fd9b7c55 100644 --- a/app/Api/ApiEntityListFormatter.php +++ b/app/Api/ApiEntityListFormatter.php @@ -106,6 +106,10 @@ class ApiEntityListFormatter */ public function format(): array { + if ($this->includeRelatedTitles) { + $this->loadRelatedTitles(); + } + $results = []; foreach ($this->list as $item) { @@ -115,6 +119,23 @@ class ApiEntityListFormatter return $results; } + /** + * Eager load the related book and chapter data when needed. + */ + protected function loadRelatedTitles(): void + { + $pages = collect($this->list)->filter(fn($item) => $item instanceof Page); + + foreach ($this->list as $entity) { + if (method_exists($entity, 'book')) { + $entity->load('book'); + } + if ($entity instanceof Page && $entity->chapter_id) { + $entity->load('chapter'); + } + } + } + /** * Format a single entity item to a plain array. */ From f60671146379e76550ce81f7a8738f848ebc63de Mon Sep 17 00:00:00 2001 From: Rashad Date: Sun, 27 Oct 2024 22:50:20 +0530 Subject: [PATCH 3/4] respective book and chapter structure added. --- app/Api/ApiEntityListFormatter.php | 23 ++--- app/Search/SearchApiController.php | 63 ++---------- dev/api/requests/search-all.http | 2 +- dev/api/responses/search-all.json | 152 ++++++++++++++++------------- tests/Api/SearchApiTest.php | 109 --------------------- 5 files changed, 103 insertions(+), 246 deletions(-) diff --git a/app/Api/ApiEntityListFormatter.php b/app/Api/ApiEntityListFormatter.php index 2fd9b7c55..7c2d09d4f 100644 --- a/app/Api/ApiEntityListFormatter.php +++ b/app/Api/ApiEntityListFormatter.php @@ -13,11 +13,6 @@ class ApiEntityListFormatter */ protected array $list = []; - /** - * Whether to include related titles in the response. - */ - protected bool $includeRelatedTitles = false; - /** * The fields to show in the formatted data. * Can be a plain string array item for a direct model field (If existing on model). @@ -79,20 +74,18 @@ class ApiEntityListFormatter /** * Enable the inclusion of related book and chapter titles in the response. */ - public function withRelatedTitles(): self + public function withRelatedData(): self { - $this->includeRelatedTitles = true; - - $this->withField('book_title', function (Entity $entity) { + $this->withField('book', function (Entity $entity) { if (method_exists($entity, 'book')) { - return $entity->book?->name; + return $entity->book()->select(['id', 'name', 'slug'])->first(); } return null; }); - $this->withField('chapter_title', function (Entity $entity) { + $this->withField('chapter', function (Entity $entity) { if ($entity instanceof Page && $entity->chapter_id) { - return optional($entity->getAttribute('chapter'))->name; + return $entity->chapter()->select(['id', 'name', 'slug'])->first(); } return null; }); @@ -106,9 +99,7 @@ class ApiEntityListFormatter */ public function format(): array { - if ($this->includeRelatedTitles) { - $this->loadRelatedTitles(); - } + $this->loadRelatedData(); $results = []; @@ -122,7 +113,7 @@ class ApiEntityListFormatter /** * Eager load the related book and chapter data when needed. */ - protected function loadRelatedTitles(): void + protected function loadRelatedData(): void { $pages = collect($this->list)->filter(fn($item) => $item instanceof Page); diff --git a/app/Search/SearchApiController.php b/app/Search/SearchApiController.php index 5072bd3b4..28a3b53e6 100644 --- a/app/Search/SearchApiController.php +++ b/app/Search/SearchApiController.php @@ -17,20 +17,9 @@ class SearchApiController extends ApiController 'query' => ['required'], 'page' => ['integer', 'min:1'], 'count' => ['integer', 'min:1', 'max:100'], - 'include' => ['string', 'regex:/^[a-zA-Z,]*$/'], ], ]; - /** - * Valid include parameters and their corresponding formatter methods. - * These parameters allow for additional related data, like titles or tags, - * to be included in the search results when requested via the API. - */ - protected const VALID_INCLUDES = [ - 'titles' => 'withRelatedTitles', - 'tags' => 'withTags', - ]; - public function __construct(SearchRunner $searchRunner, SearchResultsFormatter $resultsFormatter) { $this->searchRunner = $searchRunner; @@ -44,13 +33,6 @@ class SearchApiController extends ApiController * for a full list of search term options. Results contain a 'type' property to distinguish * between: bookshelf, book, chapter & page. * - * This method now supports the 'include' parameter, which allows API clients to specify related - * fields (such as titles or tags) that should be included in the search results. - * - * The 'include' parameter is a comma-separated string. For example, adding `include=titles,tags` - * will include both titles and tags in the API response. If the parameter is not provided, only - * basic entity data will be returned. - * * The paging parameters and response format emulates a standard listing endpoint * but standard sorting and filtering cannot be done on this endpoint. If a count value * is provided this will only be taken as a suggestion. The results in the response @@ -63,49 +45,22 @@ class SearchApiController extends ApiController $options = SearchOptions::fromString($request->get('query') ?? ''); $page = intval($request->get('page', '0')) ?: 1; $count = min(intval($request->get('count', '0')) ?: 20, 100); - $includes = $this->parseIncludes($request->get('include', '')); $results = $this->searchRunner->searchEntities($options, 'all', $page, $count); $this->resultsFormatter->format($results['results']->all(), $options); - $formatter = new ApiEntityListFormatter($results['results']->all()); - $formatter->withType(); // Always include type as it's essential for search results - - foreach ($includes as $include) { - if (isset(self::VALID_INCLUDES[$include])) { - $method = self::VALID_INCLUDES[$include]; - $formatter->$method(); - } - } - - $formatter->withField('preview_html', function (Entity $entity) { - return [ - 'name' => (string) $entity->getAttribute('preview_name'), - 'content' => (string) $entity->getAttribute('preview_content'), - ]; - }); + $data = (new ApiEntityListFormatter($results['results']->all())) + ->withType()->withTags()->withRelatedData() + ->withField('preview_html', function (Entity $entity) { + return [ + 'name' => (string) $entity->getAttribute('preview_name'), + 'content' => (string) $entity->getAttribute('preview_content'), + ]; + })->format(); return response()->json([ - 'data' => $formatter->format(), + 'data' => $data, 'total' => $results['total'], ]); } - - /** - * Parse and validate the include parameter. - * - * @param string $includeString Comma-separated list of includes - * @return array - */ - protected function parseIncludes(string $includeString): array - { - if (empty($includeString)) { - return []; - } - - return array_filter( - explode(',', strtolower($includeString)), - fn($include) => isset (self::VALID_INCLUDES[$include]) - ); - } } diff --git a/dev/api/requests/search-all.http b/dev/api/requests/search-all.http index 7fa1a304e..f9c17fa16 100644 --- a/dev/api/requests/search-all.http +++ b/dev/api/requests/search-all.http @@ -1 +1 @@ -GET /api/search?query=cats+{created_by:me}&page=1&count=2&include=titles,tags +GET /api/search?query=cats+{created_by:me}&page=1&count=2 diff --git a/dev/api/responses/search-all.json b/dev/api/responses/search-all.json index bb45b7959..f60a12f75 100644 --- a/dev/api/responses/search-all.json +++ b/dev/api/responses/search-all.json @@ -1,72 +1,92 @@ { - "data": [ - { - "id": 84, - "book_id": 1, - "slug": "a-chapter-for-cats", - "name": "A chapter for cats", - "created_at": "2021-11-14T15:57:35.000000Z", - "updated_at": "2021-11-14T15:57:35.000000Z", - "type": "chapter", - "url": "https://example.com/books/my-book/chapter/a-chapter-for-cats", - "book_title": "Cats", - "preview_html": { - "name": "A chapter for cats", - "content": "...once a bunch of cats named tony...behaviour of cats is unsuitable" - }, - "tags": [] - }, - { - "name": "The hows and whys of cats", - "id": 396, - "slug": "the-hows-and-whys-of-cats", - "book_id": 1, - "chapter_id": 75, - "draft": false, - "template": false, - "created_at": "2021-05-15T16:28:10.000000Z", - "updated_at": "2021-11-14T15:56:49.000000Z", - "type": "page", - "url": "https://example.com/books/my-book/page/the-hows-and-whys-of-cats", - "book_title": "Cats", - "chapter_title": "A chapter for cats", - "preview_html": { - "name": "The hows and whys of cats", - "content": "...people ask why cats? but there are...the reason that cats are fast are due to..." - }, - "tags": [ + "data": [ { - "name": "Animal", - "value": "Cat", - "order": 0 + "id": 84, + "book_id": 1, + "slug": "a-chapter-for-cats", + "name": "A chapter for cats", + "created_at": "2021-11-14T15:57:35.000000Z", + "updated_at": "2021-11-14T15:57:35.000000Z", + "type": "chapter", + "url": "https://example.com/books/my-book/chapter/a-chapter-for-cats", + "book": { + "id": 1, + "name": "Cats", + "slug": "cats" + }, + "preview_html": { + "name": "A chapter for cats", + "content": "...once a bunch of cats named tony...behaviour of cats is unsuitable" + }, + "tags": [] }, { - "name": "Category", - "value": "Top Content", - "order": 0 + "name": "The hows and whys of cats", + "id": 396, + "slug": "the-hows-and-whys-of-cats", + "book_id": 1, + "chapter_id": 75, + "draft": false, + "template": false, + "created_at": "2021-05-15T16:28:10.000000Z", + "updated_at": "2021-11-14T15:56:49.000000Z", + "type": "page", + "url": "https://example.com/books/my-book/page/the-hows-and-whys-of-cats", + "book": { + "id": 1, + "name": "Cats", + "slug": "cats" + }, + "chapter": { + "id": 84, + "name": "A chapter for cats", + "slug": "a-chapter-for-cats" + }, + "preview_html": { + "name": "The hows and whys of cats", + "content": "...people ask why cats? but there are...the reason that cats are fast are due to..." + }, + "tags": [ + { + "name": "Animal", + "value": "Cat", + "order": 0 + }, + { + "name": "Category", + "value": "Top Content", + "order": 0 + } + ] + }, + { + "name": "How advanced are cats?", + "id": 362, + "slug": "how-advanced-are-cats", + "book_id": 13, + "chapter_id": 73, + "draft": false, + "template": false, + "created_at": "2020-11-29T21:55:07.000000Z", + "updated_at": "2021-11-14T16:02:39.000000Z", + "type": "page", + "url": "https://example.com/books/my-book/page/how-advanced-are-cats", + "book": { + "id": 1, + "name": "Cats", + "slug": "cats" + }, + "chapter": { + "id": 84, + "name": "A chapter for cats", + "slug": "a-chapter-for-cats" + }, + "preview_html": { + "name": "How advanced are cats?", + "content": "cats are some of the most advanced animals in the world." + }, + "tags": [] } - ] - }, - { - "name": "How advanced are cats?", - "id": 362, - "slug": "how-advanced-are-cats", - "book_id": 13, - "chapter_id": 73, - "draft": false, - "template": false, - "created_at": "2020-11-29T21:55:07.000000Z", - "updated_at": "2021-11-14T16:02:39.000000Z", - "type": "page", - "url": "https://example.com/books/my-book/page/how-advanced-are-cats", - "book_title": "Cats", - "chapter_title": "A chapter for cats", - "preview_html": { - "name": "How advanced are cats?", - "content": "cats are some of the most advanced animals in the world." - }, - "tags": [] - } - ], - "total": 3 + ], + "total": 3 } diff --git a/tests/Api/SearchApiTest.php b/tests/Api/SearchApiTest.php index b80ed4530..3f2eb395c 100644 --- a/tests/Api/SearchApiTest.php +++ b/tests/Api/SearchApiTest.php @@ -2,7 +2,6 @@ namespace Tests\Api; -use BookStack\Activity\Models\Tag; use BookStack\Entities\Models\Book; use BookStack\Entities\Models\Bookshelf; use BookStack\Entities\Models\Chapter; @@ -75,112 +74,4 @@ class SearchApiTest extends TestCase $resp = $this->actingAsApiEditor()->get($this->baseEndpoint . '?query=myqueryvalue'); $resp->assertOk(); } - - public function test_all_endpoint_includes_book_and_chapter_titles_when_requested() - { - $this->actingAsApiEditor(); - - $book = $this->entities->book(); - $chapter = $this->entities->chapter(); - $page = $this->entities->newPage(); - - $book->name = 'My Test Book'; - $book->save(); - - $chapter->name = 'My Test Chapter'; - $chapter->book_id = $book->id; - $chapter->save(); - - $page->name = 'My Test Page With UniqueSearchTerm'; - $page->book_id = $book->id; - $page->chapter_id = $chapter->id; - $page->save(); - - $page->indexForSearch(); - - // Test without include parameter - $resp = $this->getJson($this->baseEndpoint . '?query=UniqueSearchTerm'); - $resp->assertOk(); - $resp->assertDontSee('book_title'); - $resp->assertDontSee('chapter_title'); - - // Test with include parameter - $resp = $this->getJson($this->baseEndpoint . '?query=UniqueSearchTerm&include=titles'); - $resp->assertOk(); - $resp->assertJsonFragment([ - 'name' => 'My Test Page With UniqueSearchTerm', - 'book_title' => 'My Test Book', - 'chapter_title' => 'My Test Chapter', - 'type' => 'page' - ]); - } - - public function test_all_endpoint_validates_include_parameter() - { - $this->actingAsApiEditor(); - - // Test invalid include value - $resp = $this->getJson($this->baseEndpoint . '?query=test&include=invalid'); - $resp->assertOk(); - $resp->assertDontSee('book_title'); - - // Test SQL injection attempt - $resp = $this->getJson($this->baseEndpoint . '?query=test&include=titles;DROP TABLE users'); - $resp->assertStatus(422); - - // Test multiple includes - $resp = $this->getJson($this->baseEndpoint . '?query=test&include=titles,tags'); - $resp->assertOk(); - } - - public function test_all_endpoint_includes_tags_when_requested() - { - $this->actingAsApiEditor(); - - // Create a page and give it a unique name for search - $page = $this->entities->page(); - $page->name = 'Page With UniqueSearchTerm'; - $page->save(); - - // Save tags to the page using the existing saveTagsToEntity method - $tags = [ - ['name' => 'SampleTag', 'value' => 'SampleValue'] - ]; - app(\BookStack\Activity\TagRepo::class)->saveTagsToEntity($page, $tags); - - // Ensure the page is indexed for search - $page->indexForSearch(); - - // Test without the "tags" include - $resp = $this->getJson($this->baseEndpoint . '?query=UniqueSearchTerm'); - $resp->assertOk(); - $resp->assertDontSee('tags'); - - // Test with the "tags" include - $resp = $this->getJson($this->baseEndpoint . '?query=UniqueSearchTerm&include=tags'); - $resp->assertOk(); - - // Assert that tags are included in the response - $resp->assertJsonFragment([ - 'name' => 'SampleTag', - 'value' => 'SampleValue', - ]); - - // Optionally: check the structure to match the tag order as well - $resp->assertJsonStructure([ - 'data' => [ - '*' => [ - 'tags' => [ - '*' => [ - 'name', - 'value', - 'order', - ], - ], - ], - ], - ]); - } - - } From fec44452cb67819b594bdfca4ca37e4a20c0f42e Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 3 Dec 2024 13:47:45 +0000 Subject: [PATCH 4/4] Search API: Updated handling of parent detail, added testing Review of #5280. - Removed additional non-needed loads which could ignore permissions. - Updated new formatter method name to be more specific on use. - Added test case to cover changes. - Updated API examples to align parent id/info in info to be representative. --- app/Api/ApiEntityListFormatter.php | 32 ++---- app/Search/SearchApiController.php | 15 +-- dev/api/responses/search-all.json | 172 ++++++++++++++--------------- tests/Api/SearchApiTest.php | 44 +++++++- 4 files changed, 142 insertions(+), 121 deletions(-) diff --git a/app/Api/ApiEntityListFormatter.php b/app/Api/ApiEntityListFormatter.php index 7c2d09d4f..3c94d96ee 100644 --- a/app/Api/ApiEntityListFormatter.php +++ b/app/Api/ApiEntityListFormatter.php @@ -2,6 +2,7 @@ namespace BookStack\Api; +use BookStack\Entities\Models\BookChild; use BookStack\Entities\Models\Entity; use BookStack\Entities\Models\Page; @@ -72,20 +73,20 @@ class ApiEntityListFormatter } /** - * Enable the inclusion of related book and chapter titles in the response. + * Include parent book/chapter info in the formatted data. */ - public function withRelatedData(): self + public function withParents(): self { $this->withField('book', function (Entity $entity) { - if (method_exists($entity, 'book')) { - return $entity->book()->select(['id', 'name', 'slug'])->first(); + if ($entity instanceof BookChild && $entity->book) { + return $entity->book->only(['id', 'name', 'slug']); } return null; }); $this->withField('chapter', function (Entity $entity) { - if ($entity instanceof Page && $entity->chapter_id) { - return $entity->chapter()->select(['id', 'name', 'slug'])->first(); + if ($entity instanceof Page && $entity->chapter) { + return $entity->chapter->only(['id', 'name', 'slug']); } return null; }); @@ -99,8 +100,6 @@ class ApiEntityListFormatter */ public function format(): array { - $this->loadRelatedData(); - $results = []; foreach ($this->list as $item) { @@ -110,23 +109,6 @@ class ApiEntityListFormatter return $results; } - /** - * Eager load the related book and chapter data when needed. - */ - protected function loadRelatedData(): void - { - $pages = collect($this->list)->filter(fn($item) => $item instanceof Page); - - foreach ($this->list as $entity) { - if (method_exists($entity, 'book')) { - $entity->load('book'); - } - if ($entity instanceof Page && $entity->chapter_id) { - $entity->load('chapter'); - } - } - } - /** * Format a single entity item to a plain array. */ diff --git a/app/Search/SearchApiController.php b/app/Search/SearchApiController.php index 28a3b53e6..79cd8cfab 100644 --- a/app/Search/SearchApiController.php +++ b/app/Search/SearchApiController.php @@ -9,21 +9,18 @@ use Illuminate\Http\Request; class SearchApiController extends ApiController { - protected SearchRunner $searchRunner; - protected SearchResultsFormatter $resultsFormatter; - protected $rules = [ 'all' => [ 'query' => ['required'], - 'page' => ['integer', 'min:1'], + 'page' => ['integer', 'min:1'], 'count' => ['integer', 'min:1', 'max:100'], ], ]; - public function __construct(SearchRunner $searchRunner, SearchResultsFormatter $resultsFormatter) - { - $this->searchRunner = $searchRunner; - $this->resultsFormatter = $resultsFormatter; + public function __construct( + protected SearchRunner $searchRunner, + protected SearchResultsFormatter $resultsFormatter + ) { } /** @@ -50,7 +47,7 @@ class SearchApiController extends ApiController $this->resultsFormatter->format($results['results']->all(), $options); $data = (new ApiEntityListFormatter($results['results']->all())) - ->withType()->withTags()->withRelatedData() + ->withType()->withTags()->withParents() ->withField('preview_html', function (Entity $entity) { return [ 'name' => (string) $entity->getAttribute('preview_name'), diff --git a/dev/api/responses/search-all.json b/dev/api/responses/search-all.json index f60a12f75..2ad896416 100644 --- a/dev/api/responses/search-all.json +++ b/dev/api/responses/search-all.json @@ -1,92 +1,92 @@ { - "data": [ + "data": [ + { + "id": 84, + "book_id": 1, + "slug": "a-chapter-for-cats", + "name": "A chapter for cats", + "created_at": "2021-11-14T15:57:35.000000Z", + "updated_at": "2021-11-14T15:57:35.000000Z", + "type": "chapter", + "url": "https://example.com/books/cats/chapter/a-chapter-for-cats", + "book": { + "id": 1, + "name": "Cats", + "slug": "cats" + }, + "preview_html": { + "name": "A chapter for cats", + "content": "...once a bunch of cats named tony...behaviour of cats is unsuitable" + }, + "tags": [] + }, + { + "name": "The hows and whys of cats", + "id": 396, + "slug": "the-hows-and-whys-of-cats", + "book_id": 1, + "chapter_id": 75, + "draft": false, + "template": false, + "created_at": "2021-05-15T16:28:10.000000Z", + "updated_at": "2021-11-14T15:56:49.000000Z", + "type": "page", + "url": "https://example.com/books/cats/page/the-hows-and-whys-of-cats", + "book": { + "id": 1, + "name": "Cats", + "slug": "cats" + }, + "chapter": { + "id": 75, + "name": "A chapter for cats", + "slug": "a-chapter-for-cats" + }, + "preview_html": { + "name": "The hows and whys of cats", + "content": "...people ask why cats? but there are...the reason that cats are fast are due to..." + }, + "tags": [ { - "id": 84, - "book_id": 1, - "slug": "a-chapter-for-cats", - "name": "A chapter for cats", - "created_at": "2021-11-14T15:57:35.000000Z", - "updated_at": "2021-11-14T15:57:35.000000Z", - "type": "chapter", - "url": "https://example.com/books/my-book/chapter/a-chapter-for-cats", - "book": { - "id": 1, - "name": "Cats", - "slug": "cats" - }, - "preview_html": { - "name": "A chapter for cats", - "content": "...once a bunch of cats named tony...behaviour of cats is unsuitable" - }, - "tags": [] + "name": "Animal", + "value": "Cat", + "order": 0 }, { - "name": "The hows and whys of cats", - "id": 396, - "slug": "the-hows-and-whys-of-cats", - "book_id": 1, - "chapter_id": 75, - "draft": false, - "template": false, - "created_at": "2021-05-15T16:28:10.000000Z", - "updated_at": "2021-11-14T15:56:49.000000Z", - "type": "page", - "url": "https://example.com/books/my-book/page/the-hows-and-whys-of-cats", - "book": { - "id": 1, - "name": "Cats", - "slug": "cats" - }, - "chapter": { - "id": 84, - "name": "A chapter for cats", - "slug": "a-chapter-for-cats" - }, - "preview_html": { - "name": "The hows and whys of cats", - "content": "...people ask why cats? but there are...the reason that cats are fast are due to..." - }, - "tags": [ - { - "name": "Animal", - "value": "Cat", - "order": 0 - }, - { - "name": "Category", - "value": "Top Content", - "order": 0 - } - ] - }, - { - "name": "How advanced are cats?", - "id": 362, - "slug": "how-advanced-are-cats", - "book_id": 13, - "chapter_id": 73, - "draft": false, - "template": false, - "created_at": "2020-11-29T21:55:07.000000Z", - "updated_at": "2021-11-14T16:02:39.000000Z", - "type": "page", - "url": "https://example.com/books/my-book/page/how-advanced-are-cats", - "book": { - "id": 1, - "name": "Cats", - "slug": "cats" - }, - "chapter": { - "id": 84, - "name": "A chapter for cats", - "slug": "a-chapter-for-cats" - }, - "preview_html": { - "name": "How advanced are cats?", - "content": "cats are some of the most advanced animals in the world." - }, - "tags": [] + "name": "Category", + "value": "Top Content", + "order": 0 } - ], - "total": 3 + ] + }, + { + "name": "How advanced are cats?", + "id": 362, + "slug": "how-advanced-are-cats", + "book_id": 13, + "chapter_id": 73, + "draft": false, + "template": false, + "created_at": "2020-11-29T21:55:07.000000Z", + "updated_at": "2021-11-14T16:02:39.000000Z", + "type": "page", + "url": "https://example.com/books/big-cats/page/how-advanced-are-cats", + "book": { + "id": 13, + "name": "Big Cats", + "slug": "big-cats" + }, + "chapter": { + "id": 73, + "name": "A chapter for bigger cats", + "slug": "a-chapter-for-bigger-cats" + }, + "preview_html": { + "name": "How advanced are cats?", + "content": "cats are some of the most advanced animals in the world." + }, + "tags": [] + } + ], + "total": 3 } diff --git a/tests/Api/SearchApiTest.php b/tests/Api/SearchApiTest.php index 3f2eb395c..9da7900ca 100644 --- a/tests/Api/SearchApiTest.php +++ b/tests/Api/SearchApiTest.php @@ -13,7 +13,7 @@ class SearchApiTest extends TestCase { use TestsApi; - protected $baseEndpoint = '/api/search'; + protected string $baseEndpoint = '/api/search'; public function test_all_endpoint_returns_search_filtered_results_with_query() { @@ -74,4 +74,46 @@ class SearchApiTest extends TestCase $resp = $this->actingAsApiEditor()->get($this->baseEndpoint . '?query=myqueryvalue'); $resp->assertOk(); } + + public function test_all_endpoint_includes_parent_details_where_visible() + { + $page = $this->entities->pageWithinChapter(); + $chapter = $page->chapter; + $book = $page->book; + + $page->update(['name' => 'name with superextrauniquevalue within']); + $page->indexForSearch(); + + $editor = $this->users->editor(); + $this->actingAsApiEditor(); + $resp = $this->getJson($this->baseEndpoint . '?query=superextrauniquevalue'); + $resp->assertJsonFragment([ + 'id' => $page->id, + 'type' => 'page', + 'book' => [ + 'id' => $book->id, + 'name' => $book->name, + 'slug' => $book->slug, + ], + 'chapter' => [ + 'id' => $chapter->id, + 'name' => $chapter->name, + 'slug' => $chapter->slug, + ], + ]); + + $this->permissions->disableEntityInheritedPermissions($chapter); + $this->permissions->setEntityPermissions($page, ['view'], [$editor->roles()->first()]); + + $resp = $this->getJson($this->baseEndpoint . '?query=superextrauniquevalue'); + $resp->assertJsonPath('data.0.id', $page->id); + $resp->assertJsonPath('data.0.book.name', $book->name); + $resp->assertJsonMissingPath('data.0.chapter'); + + $this->permissions->disableEntityInheritedPermissions($book); + + $resp = $this->getJson($this->baseEndpoint . '?query=superextrauniquevalue'); + $resp->assertJsonPath('data.0.id', $page->id); + $resp->assertJsonMissingPath('data.0.book.name'); + } }