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

Feat: Have RegexRouterRule take ordered array #1317

Merged
merged 1 commit into from
May 18, 2022
Merged

Conversation

Falx
Copy link
Contributor

@Falx Falx commented May 16, 2022

📁 Related issues

#302
#947

✍️ Description

This PR changes the Regex to ResourceStore mapping from a Map datastructure to an Array datastructure. The idea is that regexes are now tested in a fallthrough order.

This allows for more predictable routing and even allows for a catchall /.*/ regex at the end as suggested in #947. This change will however not introduce any catching/default behaviour, it will only setup the datastructure to allow this in the future. Any such change would be better introduced as a fix for #947 itself.

The first two comments, called fix: Revert: ..., are from the parent fix/#298 branch, once merged, these will be removed after a rebase. So only the latest commit should be reviewed here.

✅ PR check list

Before this pull request can be merged, a core maintainer will check whether

  • this PR is labeled with the correct semver label
    • semver.patch: Backwards compatible bug fixes.
    • semver.minor: Backwards compatible feature additions.
    • semver.major: Breaking changes. This includes changing interfaces or configuration behaviour.
  • the correct branch is targeted. Patch updates can target main, other changes should target the latest versions/* branch.
  • the RELEASE_NOTES.md document in case of relevant feature or config changes.
  • any relevant documentation was updated to reflect the changes in this PR.

@Falx Falx requested a review from joachimvh May 16, 2022 11:12
@Falx Falx changed the title Fix/#302 Feat: Have RegexRouterRule take ordered array May 16, 2022
Copy link
Member

@joachimvh joachimvh left a comment

Choose a reason for hiding this comment

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

Looks good, minor refactor comments.

config/sparql-file-storage.json Outdated Show resolved Hide resolved
src/http/representation/RepresentationPreferences.ts Outdated Show resolved Hide resolved
src/storage/routing/RegexRouterRule.ts Outdated Show resolved Hide resolved
@Falx Falx force-pushed the fix/#302 branch 3 times, most recently from bd62670 to b44c671 Compare May 17, 2022 09:47
@Falx Falx changed the base branch from versions/5.0.0 to fix/#298 May 17, 2022 09:47
@Falx Falx requested a review from joachimvh May 17, 2022 10:16
Base automatically changed from fix/#298 to versions/5.0.0 May 17, 2022 11:20
@Falx Falx added the semver.major Requires a major version bump label May 17, 2022
@joachimvh joachimvh merged commit 5399e75 into versions/5.0.0 May 18, 2022
@joachimvh joachimvh deleted the fix/#302 branch May 18, 2022 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver.major Requires a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants