Respect GET params by vytautas-pranskunas- · Pull Request #987 · angular/universal

vytautas-pranskunas- · 2018-05-15T21:06:18Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[X] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Cache is not respecting GET params

What is the new behavior?

Cache is respecting GET params

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

No matter that my API is pure JSON and i am not using GET I fixed bug that GET requests where not cached by params. Just one smal remark - for the same reason that I am not using GET i was not able to test. I tested with POST so GET should work similar. Please test befor merge.

…ith params.

googlebot · 2018-05-15T21:06:27Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

Toxicable

Can you do the following:

Undo the formatting changes. It's hard to identify what you've changed when you've reformatted the whole file.
Squash commits.
Sign the CLA
Make the commit something similar to: feat(@nguniversal/common): allow cache to work with url params

vytautas-pranskunas- · 2018-05-15T21:53:12Z

Sry do not have time for it. What I done -

joined params to object, stringify it and hashed.

constructCacheKey method ...

Also i added check to not pass request through if transferState has value for it

if (!this.isCacheActive && !this.transferState.hasKey(storeKey)) {
            // Cache is no longer active. Pass the request through.
            return next.handle(req);
        }

Added invalidation after retrieving value from transferState

if (this.transferState.hasKey(storeKey)) {
            // Request found in cache. Respond using it.
            const response = this.transferState.get(storeKey, {} as TransferHttpResponse);
            this.invalidateCacheEntry(storeKey);
...

Toxicable · 2018-05-15T22:58:29Z

+        let key = `${req.method[0]}_${req.url}`;
+
+        key = key
+            .replace(/[^\w\s]/gi, '_');


Can you explain what this is doing?
A comment should suffice here

this just beutify key. Because url can be with slashes so it replaces all not letter characters with '_'.

Toxicable · 2018-05-15T22:58:31Z

+        return key;
+    }
+
+    private hashParams(body: any) {


What is this hashing method?
Is it some well known method?
Why not use something off the shelf?

It is taken from stackowerlof. I thought why to take another dependency for such simple hashing functionality.

Can you add a comment to credit the author and source

https://stackoverflow.com/questions/7616461/generate-a-hash-from-string-in-javascript-jquery

Can you add that as a comment in your src code

Toxicable · 2018-05-15T23:16:17Z

@vytautas-pranskunas- If you cant resolve the above comments then we cannot accept this PR.

If that's ok with you then I'll implement the changes when I next get a chance

Toxicable · 2018-05-16T19:07:37Z

+                return { key: req.params.get(key) };
+            });
+
+            key += this.hashSeparator + this.hashParams(hashedParams);


Doing it this way means if someone has the params in a different order it'll casue a new request.
EG
foo=2&bar=3 produces key of 25
bar=3&foo=2 produces key of 52

When since their content is the same they should produce the same key

you are right. But usually queries are fixed ant their order not changing. But ofcourse if you want - you can sort arasm before applying hashing.

FWIW, param order should not matter. A reliable, case-sensitive (!) sort by key seems reasonable.

So you want to say that in one query it can be user=..&User=... and in another visa versa? Sounds like a smell...

The keys should be case senstive.
Meaning that the keys user and User are not equal.

I have added comment about sorting you asked

Sorry I didn't mean to make a comment about this, it needs to be applied aswell.
Merging this without the sorting will be broken.

Toxicable · 2018-06-02T22:12:36Z

Closed in favour of #1005

angular-automatic-lock-bot · 2019-09-06T00:33:00Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

KALLY\vytautas.pranskunas added 3 commits May 10, 2018 15:09

Added caching for POST requests. Fixed bug for caching GET requests w…

8408b49

…ith params.

Added POST request cache validation

33723c9

Removed POST action replay

854ab79

googlebot added the cla: no label May 15, 2018

Toxicable suggested changes May 15, 2018

View reviewed changes

Toxicable reviewed May 16, 2018

View reviewed changes

angular deleted a comment from googlebot May 16, 2018

Added comment about params sorting

26d9257

Toxicable added the state: WIP label May 20, 2018

Toxicable closed this Jun 2, 2018

Toxicable mentioned this pull request Jun 2, 2018

feat(common): TransferHttpCache now respects url params #1005

Merged

3 tasks

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 6, 2019

Conversation

vytautas-pranskunas- commented May 15, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

googlebot commented May 15, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

Toxicable left a comment

Choose a reason for hiding this comment

Uh oh!

vytautas-pranskunas- commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vytautas-pranskunas- May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Toxicable commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vytautas-pranskunas- May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Toxicable commented Jun 2, 2018

Uh oh!

angular-automatic-lock-bot Bot commented Sep 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vytautas-pranskunas- May 16, 2018 •

edited

Loading

vytautas-pranskunas- May 16, 2018 •

edited

Loading