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

URL segment validation needs clarity in predicate docs #3597

Open
chasebrewsky opened this issue Jun 15, 2020 · 4 comments
Open

URL segment validation needs clarity in predicate docs #3597

chasebrewsky opened this issue Jun 15, 2020 · 4 comments
Labels

Comments

@chasebrewsky
Copy link

chasebrewsky commented Jun 15, 2020

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

  • Place the regex matching capabilities for route matching to a more visible place in the documentation, such as the tutorial. This functionality is actually present in part 3 of the Django tutorial. You could then also place it in the generating route URLs section of the URL dispatch section. That was actually the original location I searched before I found the functionality further down the page. It's great functionality that can prevent someone from having to create a custom predicate.
  • Place a deprecation notice for the custom predicates portion of URL dispatch section and link instead to the view and route predicates section.
  • Document the predicates attribute for both the view configuration and the route configuration in the view and route predicates section. Highlight how each attribute correlates to their respective predicates, meaning that the predicates attribute for view configuration only applies to view predicates and that the predicates attribute on the route configuration only applies to route predicates (at least that's how it seems to be based on the docstrings).
  • Separate the view and route predicates section into separate sections next to each other in the same page. It both allows for an example to be shown of a route predicate and it helps to differentiates the use of the different predicates attribute between the view and route configurations.
@stevepiercy
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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.

@stevepiercy stevepiercy changed the title URL segment validation URL segment validation needs clarity in predicate docs Jul 2, 2020
@merwok
Copy link
Contributor

merwok commented Jul 2, 2020

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.

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

No branches or pull requests

4 participants