search

miguelgrinberg / microblog-api

A modern (as of 2024) Flask API back end.
MIT License
365 stars 100 forks source link

Filter users addition #10

Closed lewis-morris closed 2 years ago

lewis-morris commented 2 years ago

Added search_all_users to users blueprint (adds ability to search users) Added terms to the PagninationSchema queries and output, Updated the tests to reflect

miguelgrinberg commented 2 years ago

@lewis-morris I appreciate the PR, but this style does not really match the project well. The two main issues I have with your solution are:

  1. You are adding search options to the pagination schema. The pagination schema should be used only for pagination, if you want to add a new feature to the API, then for clarity and consistency a new schema should be used.
  2. I designed this API so that endpoints map to resources. Actions are controlled via HTTP methods. Filters are controlled with query string arguments. Adding a new endpoint just to search users does not feel like a good match.

The way I envision a search feature to work is with a new decorator such as filter_response. This decorator would take the response from the endpoint (which is a db query) and apply the filters to it, based on filtering instructions provided in query string arguments, which would have their own dedicated schema. The filtered query will then continue on to the next decorator, which would be paginated_response, where pagination would be handled on the filtered results.

lewis-morris commented 2 years ago

That's fine I appreciate your reply, that helps a lot, I just made it work rather than thinking about the bigger picture. Was just trying to get my head round what was going on by tinkering with this code and thought it was a good addition (and my first attempt at contributing to a project)

I thought I would revisit the whole react side from cover to cover after I fully understood what was going on in the backend first.

Maybe I will tweak and try again.

On Sun, 31 Jul 2022, 10:23 Miguel Grinberg, @.***> wrote:

@lewis-morris https://github.com/lewis-morris I appreciate the PR, but this style does not really match the project well. The two main issues I have with your solution are:

  1. You are adding search options to the pagination schema. The pagination schema should be used only for pagination, if you want to add a new feature to the API, then for clarity and consistency a new schema should be used.
  2. I designed this API so that endpoints map to resources. Actions are controlled via HTTP methods. Filters are controlled with query string arguments. Adding a new endpoint just to search users does not feel like a good match.

— Reply to this email directly, view it on GitHub https://github.com/miguelgrinberg/microblog-api/pull/10#issuecomment-1200386244, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIVZ7JE4IRVH2FG6QLQZLTLVWZAZXANCNFSM55ERNFFQ . You are receiving this because you were mentioned.Message ID: @.***>

lewis-morris commented 2 years ago

Ignore this, I didn't see all your response.

lewis-morris commented 2 years ago

@lewis-morris I appreciate the PR, but this style does not really match the project well. The two main issues I have with your solution are:

  1. You are adding search options to the pagination schema. The pagination schema should be used only for pagination, if you want to add a new feature to the API, then for clarity and consistency a new schema should be used.
  2. I designed this API so that endpoints map to resources. Actions are controlled via HTTP methods. Filters are controlled with query string arguments. Adding a new endpoint just to search users does not feel like a good match.

The way I envision a search feature to work is with a new decorator such as filter_response. This decorator would take the response from the endpoint (which is a db query) and apply the filters to it, based on filtering instructions provided in query string arguments, which would have their own dedicated schema. The filtered query will then continue on to the next decorator, which would be paginated_response, where pagination would be handled on the filtered results.

This is what I thought, cheers. Makes sense of course when you know how.