Pylons / pyramid

Pyramid - A Python web framework
https://trypyramid.com/
Other
3.97k stars 887 forks source link

URL segment validation needs clarity in predicate docs #3597

Open chasebrewsky opened 4 years ago

chasebrewsky commented 4 years ago

I was trying to figure out how to match a URL segment to a UUID value today to prevent unintended route matching. I had the route /companies/{uuid} and the route /companies/{uuid}/edit and a broken route path was causing the application to go to /companies/edit. This was causing backend issues since there was no UUID validation on the route itself.

The documentation for doing this is conflicting, hard to find, and missing some helpful functionality.

The conflicting portion of the documentation is here. It mentions the use of custom_predicates which has actually been deprecated since 1.5.

It seems that the preferred method of performing this validation is via route predicates. This was hard to find because I originally associated this page more-so with application middleware instead of route matching. I can understand why it was put on this page after reading it because the route predicates act as a form of routing middleware, but it wasn't immediately obvious when trying to perform a google search.

The documentation is also missing the fact that you can define predicate usage using the predicates attribute on the add_route method for the configurator. It's actually incredibly useful because it seems you can define multiple predicate values with the same predicate this way, but it's buried in the docstrings.

Suggestions

stevepiercy commented 4 years ago

Thank you. Your suggestions sound reasonable to me. I'd like another opinion from a core contributor before you invest your time to start a PR for review, just in case there is something I don't fully understand. @mmerickel @bertjwregeer

mmerickel commented 4 years ago

I think part of the complexity is that I think almost all of the standard predicates are available to both views and routes and so we like to document those in one spot. I agree that there's lots of problems with how that is structured right now however and welcome improvements. I totally agree that the docs are unclear about route predicates versus view predicates.

stevepiercy commented 4 years ago

I have a few more thoughts and suggestions.

  1. This was hard to find because I originally associated this page more-so with application middleware instead of route matching.

    Newbie Steve agrees. We don't define terms well before they are used. The page title is "Using Hooks". Newbie Steve thinks of "webhooks" from GitHub or Mailchimp, and that is a wrong guess. There is no term "hook" in the Glossary to help enlighten the reader. Also the introductory sentence is vague and does not help the user understand what "using hooks" really means. What is a hook? ¯\_(ツ)_/¯ Does it have something to do with hook_zca? @mmerickel can you help a brother out?

    The section headings on this page also do not help the developer find what they seek. If I'm looking for "route matching" or "match url segment", the first or second hit is "URL Dispatch" which contains the deprecated "Custom Route Predicates", as you found. These headings could be more helpful.

  2. We define 4 terms in the Glossary that include "predicate", but we omit 1 term that is used on this page:

  3. We do not want to have the same content maintained in multiple locations. I would prefer a single authoritative source of narrative content, and have tutorials refer to that content as needed. Also I do not want to duplicate and maintain docstrings in narrative documentation, so linking to that as an authoritative source is appropriate.

With that I'll try to summarize a task list. Feedback appreciated.

  1. Decide where to put the authoritative narrative content. Does it belong in "Using Hooks", another existing page, or its own new page? Currently it is located in the section View and Route Predicates under "Using Hooks". IMO, URL Dispatch makes the most sense to me, but that's likely because I don't know the definition of a "hook" and don't know any better.
  2. Break apart the single view and route predicates section into two separate sections, one after the other, in the same page.
  3. Add content for regex matching of routes to the authoritative narrative content.
  4. Add links in the authoritative narrative content for routes and views to their respective Pyramid API docstrings pyramid.config.Configurator.add_route and pyramid.config.Configurator.add_view.
  5. Place a deprecation notice for the section Custom Route Predicates under "URL Dispatch", and link to the authoritative content.
  6. Where relevant in tutorials, refer to regex matching of routes and view and route predicates in the authoritative narrative docs.
  7. Define terms in the Glossary, "hook" and "subscriber predicates".
  8. Apply :term: to terms at least in their first appearance in a page, and wherever else is helpful.
  9. Clarify section headings by following a slightly more verbose pattern of "How to do action using thing". For example, "View and Route Predicates", could be rephrased to "How to match URLs to routes with route predicates" and "How to match call view code with view predicates".
  10. Add :index: entries to section headings for better cross-referencing.

The last four items will also improve the search index generated by Sphinx and in third party search engines.

merwok commented 4 years ago

I have recently changed a predicate and added a new one, and it wasn’t great to add documentation in three spots. Mark me interested in improving the situation.