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 limits to the number of router rule registration #1752

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sisidovski
Copy link
Contributor

@sisidovski sisidovski commented Jan 30, 2025

In #1746, we discussed if the spec should give guidelines about the limit of added routes, and how the user agent should behave if the added routes exceeds the limit.

We have an agreement that we're willing to pick some specific limits. This PR proposes 1024 as the total number of router conditions, and 10 as the max depth of router condition nesting level. If it exceeds those limits, the user agent throws a TypeError.

For the counting conditions part, let me clarify how we calculate the total count as this a bit tricky.

Assuming that router conditions can be nested by using or and not syntaxes. We count every sub router conditions inside the parent or or not condition, including the parent condition itself.

Here are some examples.

self.addEventListener('install', event => {
  // The condition count of this addRoutes() is 1.
  //
  // The max nesting level is 1.
  event.addRoutes({
    condition: {
      urlPattern: { pathname: "/form/*" },
    },
    source: "network"
  });

  // The condition count of this addRoutes() is 1.
  // Even though there are multiple fields in one router condition, we treat is as 1.
  //
  // The max nesting level is 1.
  event.addRoutes({
    condition: {
      urlPattern: { pathname: "/form/*" },
      requestMethod: "post",
      requestMode: "navigation",
      runningStatus: "running"
    },
    source: "network"
  });

  // The condition count of this addRoutes() is 2.
  //
  // The max nesting level is 1.
  event.addRoutes([
    {
      condition: { requestMethod: "post" },
      source: "network"
    },
    {
      condition: { requestMethod: "get" },
      source: "network"
    }
  ]);

  // The condition count of this addRoutes() is 4.
  // `or` condition itself is inluded in the total count.
  //
  // The max nesting level is 2.
  event.addRoutes([
    {
      condition: {
        or: [
          { urlPattern: { pathname: "/form/*" } },
          { requestMethod: "post" },
          { requestMode: "navigate" },
        ]
      },
      source: "network"
    }
  ]);

  // The condition count of this addRoutes() is 7.
  // 3 `or` + 1 `not` + 1 `requestMethod` + 1 `requestMode` + 1 `runningStatus` = 7
  //
  // The max nesting level is 5.
  event.addRoutes([
    {
      condition: {
        or: [
          {
            or: [{
              or: [
                { requestMethod: 'get' },
                {
                  not: {
                    requestMode: 'navigate'
                  }
                }
              ]
            }]
          },
          { runningStatus: 'running' }
        ]
      },
      source: "network"
    }
  ]);

  // In total, 15 router conditions are registered in the install event.
  // Max nesting level is 5.
});

#1714 will be migrated into this PR.


Preview | Diff

docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
@sisidovski sisidovski force-pushed the limit branch 2 times, most recently from b0af1d0 to f427b1a Compare January 31, 2025 01:53
@sisidovski sisidovski changed the title Add 1024 as the number of router rule registration limit Add limits to the number of router rule registration Jan 31, 2025
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
docs/index.bs Outdated Show resolved Hide resolved
1. If |result|'s [=count router condition result/depth=] exceeds |maxDepth|, return |result|.
1. If |condition|["{{RouterCondition/_or}}"] [=map/exists=], then:
1. Increment |result|'s [=count router condition result/depth=] by one.
1. For each |orCondition| of |condition|["{{RouterCondition/_or}}"]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that the number of conditions under |orCondition| may not be counted?
e.g.
or: [{singleRule}, {singleRule}, {singleRule}]
|result| might be overwritten by the result for {singleRule} for three times, but we want |result|'s [=total count=] to be three.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe those are correctly counted? Let's walk through the current algorithm. btw, under the current design, or: [{singleRule}, {singleRule}, {singleRule}], this total count is expected to be four (or should be counted as one condition).

  1. We call count-router-inner-conditions with the root or condition. The total count will be one.
  2. Starts the for loop with the first {singleRule}.
  3. In the first sub-step, |result|'s total count will be two, because we pass "one" as total count to count-router-inner-conditions in this sub-step. total count will be incremented inside the algorithm.
  4. The second {singleRule} is handled. total count will be three.
  5. The third {singleRule} is handled. total count will be four.

Am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon the sync, I understand how this works.
The last result will be the |currentResult|'s total count, and it will be passed to the coming count-router-inner-conditions. Therefore, the value will be updated like:

  1. |currentResult| is passed to count-router-inner-conditions's argument
  2. in the count-router-inner-conditions, the given argument will be updated
  3. the updated value will be returned
  4. |currentResult|'s total count will be set to the returned result's total count.

Therefore, even if the algorithm does not have add x + y like operation, the count will be updated.
I feel the data flow is difficult to track at once, and hope some notes to explain that as you have already noted below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated to set |result| to be the result of count-router-inner-conditions directly, and now we don't have to set total count explicitly. Hence note is not required anymore. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?
After reviewing this, I got familiar with the algorithm well and can easily read this like so.
However, I am not sure how people first look feel on this. Let me delegate the decision to other reviewers on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The algorithm seems pretty intuitive to me now without needing a note. It is clear that result is mutated and the for-loop over all _or conditions means it will be mutated once for each sub-condition.

docs/index.bs Outdated Show resolved Hide resolved
@yoshisatoyanagisawa
Copy link
Collaborator

Sorry for the recent update, but will you resolve the conflicts?

@sisidovski
Copy link
Contributor Author

Sorry for the recent update, but will you resolve the conflicts?

Done

@yoshisatoyanagisawa
Copy link
Collaborator

Thank you. lgtm.

@yoshisatoyanagisawa
Copy link
Collaborator

@youennf @asutherland @domenic Will you take a look at the PR?

@yoshisatoyanagisawa
Copy link
Collaborator

Let me wait for a few days to gather thoughts from who joined #1746 before merge.

docs/index.bs Outdated Show resolved Hide resolved
1. If |result|'s [=count router condition result/depth=] exceeds |maxDepth|, return |result|.
1. If |condition|["{{RouterCondition/_or}}"] [=map/exists=], then:
1. Increment |result|'s [=count router condition result/depth=] by one.
1. For each |orCondition| of |condition|["{{RouterCondition/_or}}"]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The algorithm seems pretty intuitive to me now without needing a note. It is clear that result is mutated and the for-loop over all _or conditions means it will be mutated once for each sub-condition.

Co-authored-by: Domenic Denicola <[email protected]>
Copy link

@asutherland asutherland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

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

Successfully merging this pull request may close these issues.

4 participants