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

Invert "does this entry match the filter" logic #947

Merged

Conversation

EvanHahn
Copy link
Contributor

To be merged into #940.

Before this change, we turned filters into a list of path prefixes and then checked entry paths against those prefixes. Rough pseudocode:

function doesEntryMatchFilter({ path }, filter) {
  const pathPrefixes = pathPrefixesFromFilter(filter)
  return pathPrefixes.some((prefix) => path.startsWith(prefix))
}

For performance and simplicity, I think it's cleaner if we "look up" entry paths in the existing filter object. Rough pseudocode:

function doesEntryMatchFilter({ path }, filter) {
  const { type, variant } = parsePath(path)
  return filter[type]?.includes(variant)
}

I think this has two advantages:

  • It's less code. We don't need to worry about de-duping paths (e.g., /photos/original versus /photos/). We don't need to worry about sketchy paths (e.g., /photos/garbage/../original).
  • It's faster (at least, in theory). Rather than having to iterate over every path prefix, we only need to iterate over the variants of each path. (This could be further optimized with a Set.)

Before this change, we turned filters into a list of path prefixes and
then checked entry paths against those prefixes. Rough pseudocode:

```javascript
function doesEntryMatchFilter({ path }, filter) {
  const pathPrefixes = pathPrefixesFromFilter(filter)
  return pathPrefixes.some((prefix) => path.startsWith(prefix))
}
```

For performance and simplicity, I think it's cleaner if we "look up"
entry paths in the existing filter object. Rough pseudocode:

```javascript
function doesEntryMatchFilter({ path }, filter) {
  const { type, variant } = parsePath(path)
  return filter[type]?.includes(variant)
}
```

I think this has two advantages:

- It's less code. We don't need to worry about de-duping paths (e.g.,
  `/photos/original` versus `/photos/`). We don't need to worry about
  sketchy paths (e.g., `/photos/garbage/../original`).
- It's faster (at least, in theory). Rather than having to iterate over
  every path prefix, we only need to iterate over the variants of each
  path. (This could be further optimized with a `Set`.)
@EvanHahn EvanHahn merged commit b1cc628 into chore/entries-stream Oct 30, 2024
8 checks passed
@EvanHahn EvanHahn deleted the 940-use-blob-filter-instead-of-path-prefix branch October 30, 2024 16:15
EvanHahn added a commit that referenced this pull request Nov 5, 2024
Before this change, we turned filters into a list of path prefixes and
then checked entry paths against those prefixes. Rough pseudocode:

```javascript
function doesEntryMatchFilter({ path }, filter) {
  const pathPrefixes = pathPrefixesFromFilter(filter)
  return pathPrefixes.some((prefix) => path.startsWith(prefix))
}
```

For performance and simplicity, I think it's cleaner if we "look up"
entry paths in the existing filter object. Rough pseudocode:

```javascript
function doesEntryMatchFilter({ path }, filter) {
  const { type, variant } = parsePath(path)
  return filter[type]?.includes(variant)
}
```

I think this has two advantages:

- It's less code. We don't need to worry about de-duping paths (e.g.,
  `/photos/original` versus `/photos/`). We don't need to worry about
  sketchy paths (e.g., `/photos/garbage/../original`).
- It's faster (at least, in theory). Rather than having to iterate over
  every path prefix, we only need to iterate over the variants of each
  path. (This could be further optimized with a `Set`.)
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.

2 participants