馃敟 Feature: migrate redirect and rewrite Rules maps to RuleList - #325
Draft
ReneWerner87 wants to merge 4 commits into
Draft
馃敟 Feature: migrate redirect and rewrite Rules maps to RuleList#325ReneWerner87 wants to merge 4 commits into
ReneWerner87 wants to merge 4 commits into
Conversation
Both middlewares deprecated Rules map[string]string in favour of an ordered RuleList, so one migration serves them: their new fields are spelled the same and the reason is the same. The entries are emitted in the order the deprecated map ranked them rather than the order they were written. A list is tried top to bottom, so a config that happened to spell the catch-all first would otherwise start answering different paths after migrating. A map this cannot read verbatim, a variable or a computed value, is left alone: it still compiles against the deprecated field, which is better than a guess.
It was in the blanket v3 set, so migrating to 3.5.0 would have written a field that release does not have. Its own range now, with the target constraint carrying the version the field landed in. The import match also had to widen past v3: the package-path migration runs first, so by the time this one reads the file a v4 target has already moved the import.
The previous range read ">=2.0.0-0" to ">=3.6.0-0", which also matched a v4 target, since 4.0.0 satisfies ">=3.6.0-0". It is a v3 change: RuleList lands in 3.6.0 and Rules keeps working beside it for the rest of v3. With a v4 target out of scope the import match goes back to /v3/, since the package-path migration can no longer have moved it first.
Asking for 3.6.0 is what should trigger it, whatever the config is on today,
so the band belongs on the target constraint: ">=3.6.0-0 <4.0.0-0". The two
ranges before this one had it on the wrong side, first matching every v3
target and then only a config already past 3.6.0.
Testing a real v2 origin end to end turned up a second bug: the entry parser
walked lines, so "Rules: map[string]string{"/a": "/b", "/c": "/d"}" on one
line read as unparseable and was left behind. It walks the body now and
checks that only separators and comments sit between the pairs, which is
still how a computed key or value declines the rewrite.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Draft, because it depends on two fiber PRs that are still open: gofiber/fiber#4621 (redirect) and gofiber/fiber#4633 (rewrite). Both deprecate
Rules map[string]stringin favour of an orderedRuleList []Rule, and both keep the map working for the whole of v3, so nothing here is urgent. Ready to un-draft once they land.One migration serves both middlewares: the new field is spelled the same in each and the reason for the deprecation is the same.
The ordering is the point
Note which rule comes first above. The entries are emitted in the order the deprecated map ranked them, not the order they were written.
A map has no order, so the middleware sorted its keys: most path text before the first
*, then most path text overall, then fewest asterisks, then the key. ARuleListis tried top to bottom. Copying the source order across would therefore be a silent behaviour change for any config that happened to spell a catch-all above something it also matches, which is exactly the case a user is least likely to notice.sortRulesreproduces the middleware's ranking so each path is answered the way it was before.What it does not touch
A
Rulesvalue this cannot read verbatim is left alone rather than guessed at, and prints nothing:Rules: rules"/old": targetConfigthat happens to have aRulesfieldAll three still compile against the deprecated field, which is a better outcome than a wrong rewrite. Comments and blank lines inside the map are skipped; anything else in there stops the rewrite for that block.
Import aliases are honoured, so
rd "github.com/gofiber/fiber/v3/middleware/redirect"emits[]rd.Rule.Registration
Its own range, inside v3 only:
{ From: ">=3.6.0-0", To: "<4.0.0-0", Functions: []MigrationFn{v3migrations.MigrateRuleList}, }RuleListlands in 3.6.0 andRuleskeeps working beside it for the rest of v3, so that is the band where this applies.Test_DoMigration_RuleListRunsWithinV3From36pins it: 3.6.0 and later within v3 migrate, 3.5.0 and a v2 origin do not, and a 4.0.0 target does not.It is idempotent, so a repeat run reports nothing.
Tests
Test_MigrateRuleList_*covers redirect, rewrite, the ranking (asserting the specific rule is emitted above the catch-all), import aliases, the three shapes it declines, and idempotency. Full suite passes.