Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add book and chapter titles to search API results #5280

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

rashadkhan359
Copy link
Contributor

Purpose

Add the ability to include book and chapter titles in the /api/search endpoint results through an optional include parameter. This makes it easier for API consumers to show the full content hierarchy context for search results without additional API calls.

Closes

API Request #5140

Implementation

New Features

  • Added optional include parameter to /api/search endpoint
  • Supported include options:
    • titles - Includes book_title and chapter_title for applicable entities
    • tags - Include associated tags (existing functionality now controlled via include)

Multiple includes can be combined using comma separation.(if we want to extend on it.)

Changes

  • Added new ApiEntityListFormatter::withRelatedTitles(), ApiEntityListFormatter::loadRelatedTitles() and SearchApiController::parseIncludes() method.
  • Modified following existing methods:
    • SearchApiController::$rules
    • SearchApiController::all()
    • ApiEntityListFormatter::format()
  • Added support for modular includes in search API controller
  • Added comprehensive tests for new functionality

Technical Details

  • Uses eager loading to prevent N+1 queries
  • Might not maintain backward compatibility (specifically since tags were already included by default but now we need to pass it in include for it to be included. This behavior if problematic can be altered easily.)
  • Follows existing BookStack API patterns
  • Includes full test coverage

Testing

New test cases cover:

  • Including/excluding titles
  • Multiple include combinations
  • Input validation
  • Edge cases

All existing tests pass without modification.

Documentation

The API documentation has been edited as well to cater to the new changes.

Example Request

GET /api/search?query=example&include=titles,tags

Example Response

{
  "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 <strong>cats</strong>",
        "content": "...once a bunch of <strong>cats</strong> named tony...behaviour of <strong>cats</strong> 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 <strong>cats</strong>",
        "content": "...people ask why <strong>cats</strong>? but there are...the reason that <strong>cats</strong> 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_title": "Cats",
      "chapter_title": "A chapter for cats",
      "preview_html": {
        "name": "How advanced are <strong>cats</strong>?",
        "content": "<strong>cats</strong> are some of the most advanced animals in the world."
      },
      "tags": []
    }
  ],
  "total": 3
}

PS

This is my first-ever contribution to open source projects. I kindly ask for a thorough review, and I'm eager to improve upon any mistakes I may have made.

@ssddanbrown
Copy link
Member

Thanks for offering this @rashadkhan359!

I'm happy to include these properties, since it reflects what we already show in the BookStack search UI.
I'd rather not go down the path of optional data via the include tag though, as I'd prefer to keep the scope of what's provided in the API aligned with our UI and what's sensible, otherwise our scope could go wild with adding extra include tags to suit all potential use-cases.
Therefore please could you remove the include system and just include these properties for this specific response.

Also,, rather than new custom properties like book_title I'd prefer if we just loaded book and chapter properties to allow multiple properties (limited to just id, name and slug) of those elements to be shown where needed, like so:

{
// ...
  "book": {
    "id": 4,
    "name": "My great book",
    "slug": "my-great-book"
  }
// ...
}

@rashadkhan359
Copy link
Contributor Author

Hello @ssddanbrown, Thank you for the wonderful and insightful feedback. I've made the requested changes and updated the docs as well accordingly.

ssddanbrown added a commit that referenced this pull request Dec 3, 2024
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.
@ssddanbrown ssddanbrown merged commit f606711 into BookStackApp:development Dec 3, 2024
1 check failed
@ssddanbrown ssddanbrown added this to the Next Feature Release milestone Dec 3, 2024
@ssddanbrown
Copy link
Member

Thanks @rashadkhan359,
This is now merged to be part of the next feature release.

I followed this up with fec4445 to add testing and to tweak implementation a bit, to adhere to permissions and to prevent potential duplicate loading effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants