Check url with locale prefixes and compare with redirectSlug #16

ValJed · 2023-07-24T16:01:27Z

Summary

Modifies the middleware to check the originalUrl instead of url in which the locale prefix has been removed.
Removes the regex and performs a $or request on redirect pieces redirectSlug property because there was issues when using the slug since this one can be modified by the server for de duplication for example.
When redirecting to internal pages, use the page piece slug property instead of the _url of the new page to avoid being redirected on pages like /fr/fr/page.

What are the specific steps to test this change?

On project with multiple locales using prefixes, when creating a new redirection, you must specify the locale prefix, it allows to avoid conflicts with potential pieces with the same slug in other locales.

What kind of change does this PR introduce?

Make sure the PR fulfills these requirements:

It includes a) the existing issue ID being resolved, b) a convincing reason for adding this feature, or c) a clear description of the bug it resolves
The changelog is updated
Related documentation has been updated
Related tests have been updated

…ting to redirectSlug and not page url

linear · 2023-07-24T16:01:30Z

PRO-4493 Redirect/Localization Issues: Prefixes are removed before consulting redirects

When we are using the redirect module on a site on which we have locale prefixes, it does not work as intended. When you want to redirect a French page like /fr/auto, you cannot because if /auto exists in the EN locale, it will remove the prefix before consulting the redirects.

https://gitlab-dcadcx.michelin.net/pa/apostrophe-enhancements/-/issues/1223

ValJed · 2023-07-27T15:37:15Z

index.js

+          const status = (parsedCode && !isNaN(parsedCode)) ? parsedCode : 302;
+
+          if (target.urlType === 'internal' && target._newPage && target._newPage[0]) {
+            return req.res.redirect(status, target._newPage[0].slug);


Modified to take target._newPage[0].slug instead of target._newPage[0]._url because I was ended with redirection to pages like /fr/fr/page since the locale prefix is automatically added.
Didn't see potential issues by using slug.

Use rawRedirect.

ValJed · 2023-07-27T15:38:58Z

index.js

-              if (isNaN(status) || !status) {
-                status = 302;
-              }
+          const target = results.find(({ redirectSlug }) => redirectSlug === slug) ||


Same logic as before here just condensed and avoid two useless iterations (some in the if statements).

ValJed · 2023-07-27T15:43:12Z

index.js

-            }
+          const slug = req.originalUrl;
+          const [ pathOnly ] = slug.split('?');
+          const results = await self.find(req, { $or: [ { redirectSlug: slug }, { redirectSlug: pathOnly } ] }).toArray();


I get redirections with and without query params. Then I still perform a find on both depending on the ignoreQueryString property.
I don't think we need a regex here, or maybe just to remove the last slash?
BTW using redirectSlug instead of slug here because with the latter it doesn't work when slug is deduplicated, by an archived piece for example.

I'm saying "uncle" here. Using slug for speed is just too messy. So please add a mongodb database index on redirectSlug. See the doc module for examples.

Uncle? Ok doing that.

boutell · 2023-07-27T16:50:26Z

index.js

-            }
+          const slug = req.originalUrl;
+          const [ pathOnly ] = slug.split('?');
+          const results = await self.find(req, { $or: [ { redirectSlug: slug }, { redirectSlug: pathOnly } ] }).toArray();


I'm saying "uncle" here. Using slug for speed is just too messy. So please add a mongodb database index on redirectSlug. See the doc module for examples.

boutell · 2023-07-27T16:51:23Z

index.js

+          const status = (parsedCode && !isNaN(parsedCode)) ? parsedCode : 302;
+
+          if (target.urlType === 'internal' && target._newPage && target._newPage[0]) {
+            return req.res.redirect(status, target._newPage[0].slug);


Use rawRedirect.

boutell · 2023-07-27T16:52:18Z

index.js

+          if (target.urlType === 'internal' && target._newPage && target._newPage[0]) {
+            return req.res.redirect(status, target._newPage[0].slug);
+          } else if (target.urlType === 'external' && target.externalUrl.length) {
+            return req.res.redirect(status, target.externalUrl);


rawRedirect here too.

boutell · 2023-08-03T16:36:51Z

Reminder that this request for changes is pending.

boutell · 2023-08-14T20:45:00Z

CHANGELOG.md

@@ -1,5 +1,9 @@
 # Changelog

+## Unreleased
+
+- Fixes redirections when using locales prefixes. The locale prefix must be added to the `redirectSlug` to avoid conflicts with other locales.


locale prefixes

boutell · 2023-08-14T20:47:14Z

index.js

+          const slug = req.originalUrl;
+          const [ pathOnly ] = slug.split('?');
+          const results = await self
+            .find(req, { $or: [ { redirectSlug: slug }, { redirectSlug: pathOnly } ] })


This may be a slight performance hit, but OK. Time to make this code simple.

You think it's less efficient than a regex query?
I added an index on redirectSlug also.

ValJed added 2 commits July 24, 2023 17:49

improving code, checking originalUrl (with locale prefix) and redirec…

8b47007

…ting to redirectSlug and not page url

adds changelog

67aefd9

ValJed self-assigned this Jul 24, 2023

ValJed marked this pull request as draft July 24, 2023 16:01

ValJed added 3 commits July 26, 2023 15:25

redirects to slug

4743bf6

finds on redirectSlug

d7bbd62

makes $or request on slug and pathOnly

5015788

ValJed commented Jul 27, 2023

View reviewed changes

ValJed requested a review from boutell July 27, 2023 16:16

ValJed marked this pull request as ready for review July 27, 2023 16:20

boutell requested changes Jul 27, 2023

View reviewed changes

uses rawRedirect, creates indexes on redirectSlug

43626ca

ValJed requested a review from boutell August 14, 2023 09:05

boutell requested changes Aug 14, 2023

View reviewed changes

fixes typo

07b340a

ValJed requested a review from boutell August 16, 2023 12:38

uses _url with rawRedirect for internal redirects

94d01b1

boutell approved these changes Aug 16, 2023

View reviewed changes

ValJed merged commit 7a09dfb into main Aug 16, 2023

ValJed deleted the pro-4493-fix-locale-with-prefix branch August 16, 2023 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check url with locale prefixes and compare with redirectSlug #16

Check url with locale prefixes and compare with redirectSlug #16

ValJed commented Jul 24, 2023 •

edited

Loading

linear bot commented Jul 24, 2023

ValJed Jul 27, 2023 •

edited

Loading

boutell Jul 27, 2023

ValJed Aug 14, 2023

ValJed Jul 27, 2023

ValJed Jul 27, 2023

boutell Jul 27, 2023

ValJed Aug 14, 2023

boutell Jul 27, 2023

boutell Jul 27, 2023

boutell Jul 27, 2023

ValJed Aug 14, 2023

boutell commented Aug 3, 2023

boutell Aug 14, 2023

boutell Aug 14, 2023

ValJed Aug 16, 2023

Check url with locale prefixes and compare with redirectSlug #16

Check url with locale prefixes and compare with redirectSlug #16

Conversation

ValJed commented Jul 24, 2023 • edited Loading

Summary

What are the specific steps to test this change?

What kind of change does this PR introduce?

Make sure the PR fulfills these requirements:

linear bot commented Jul 24, 2023

ValJed Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boutell commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValJed commented Jul 24, 2023 •

edited

Loading

ValJed Jul 27, 2023 •

edited

Loading