Follow up #1172 #1849: Fix cache issue. IP filtering helpers #1221

nurhat · 2020-05-05T14:42:28Z

Proposed Changes

Add content language header to CacheKeyGenerator
IP filtering tries to find exact client (proxy, firewall, loadbalance)
Schema can be modified by a service discovery provider
Cache only if HTTP status is OK

EngRajabi · 2020-05-24T08:20:13Z

#1172

garybond · 2021-02-10T11:37:37Z

Any update on when we might get this?
Been caught out recently by: 'cache only if http status is OK'

Cherry picked from ThreeMammals#1221

raman-m · 2023-07-19T19:53:11Z

@nurhat Hi!
Thanks for the great PR and your interest in Ocelot!

What issue is this PR related to?

Could you work on merge conflicts please?
It seems your fork too old. Go to your fork and merge open PRs to sync your fork please!

raman-m · 2023-07-28T17:46:20Z

@nurhat
I've rebased feature branch onto ThreeMammals:develop.
Because you use develop branch as feature one, therefore in future you will not be able to create a new feature branch from develop one because it has a diff, 7 commits ahead of ThreeMammals:develop.

Wish you good luck during code review!

And once again,
Maybe, this PR is related to an issue in backlog... Which one?

raman-m · 2023-07-28T17:50:09Z

@EngRajabi commented on May 24, 2020

Mohsen, could you review the code please?

raman-m · 2023-07-28T17:52:13Z

@garybond commented on Feb 10, 2021:

Any update on when we might get this?

Coming soon... 😃
Your code review is welcome! 😉

RaynaldM · 2023-07-31T05:38:36Z

src/Ocelot/Security/SecurityPolicyExtensions.cs

+        return string.Empty;
+    }
+
+    public static List<string> SplitCsv(this string csvList, bool nullOrWhitespaceInputReturnsNull = false)


could be private

...till the moment the author will write at least one unit test for this method. 🤣

unless no one uses it outside of this class

RaynaldM · 2023-07-31T05:39:22Z

src/Ocelot/Security/SecurityPolicyExtensions.cs

+        return string.Empty;
+    }
+
+    public static List<string> SplitCsv(this string csvList, bool nullOrWhitespaceInputReturnsNull = false)


Suggested change

public static List<string> SplitCsv(this string csvList, bool nullOrWhitespaceInputReturnsNull = false)

private static IReadOnlyCollection<string> SplitCsv(this string csvList, bool nullOrWhitespaceInputReturnsNull = false)

Ensuring about strong safety by IReadOnlyCollection and making the method private don't give much outcome by having it as read-only.

this method is only use in this class, so, it must be private. There will always be time to make it public, if necessary

RaynaldM · 2023-07-31T05:43:29Z

src/Ocelot/Security/SecurityPolicyExtensions.cs

+    {
+        if (string.IsNullOrWhiteSpace(csvList))
+        {
+            return nullOrWhitespaceInputReturnsNull ? null : new List<string>();


Suggested change

return nullOrWhitespaceInputReturnsNull ? null : new List<string>();

return nullOrWhitespaceInputReturnsNull ? null : Array.Empty<string>();

RaynaldM · 2023-07-31T05:43:57Z

src/Ocelot/Security/SecurityPolicyExtensions.cs

+            .Split(',')
+            .AsEnumerable()
+            .Select(s => s.Trim())
+            .ToList();


Suggested change

.ToList();

.ToArray();

@RaynaldM
Why to array?
The author just wants to match the returning type which is List<string>😄
Are you kidding? 😉 He will get compilation error. 🤣

if @nurhat applies all these changes no problems compiling.

I always recommend the use of Single-Dimensional Array because it is the most sober in memory and cpu.
If we don't need more functions, like in lists and other dictionaries, then let's use the simpler and more sober one

src/Ocelot/Security/SecurityPolicyExtensions.cs

raman-m · 2023-07-31T17:44:11Z

@RaynaldM Ray,
Thanks for your code review! 🤝

wast · 2023-07-31T20:06:34Z

src/Ocelot/Cache/Middleware/OutputCacheMiddleware.cs

+            }
+            else
+            {
+                Logger.LogDebug($"http request failed,  could not create cache for {downstreamUrlKey}");


There's a double space. Also message could be better, something like "http request failed, skipped caching for {downstreamUrlKey}"

src/Ocelot/Security/SecurityPolicyExtensions.cs

… issues etc)

raman-m · 2024-11-11T09:32:33Z

@EngRajabi commented on May 24, 2020

But both #1172 and #1849 are closed?!

raman-m · 2024-11-11T09:45:36Z

@nurhat, are you available? The following issues need attention:

This PR should be rebased onto the develop branch after pressing the Sync fork button.
It appears caching has been addressed in both Cache by header value: a new Header property in (File)CacheOptions configuration of a route #1172 and #1172 Default CacheKeyGenerator #1849, so the caching-related code needs to be removed.
The proposed SecurityPolicyExtensions should be integrated with both the Security and Placeholders features.

Should we agree to close this PR, or do you wish to keep it open for further rebase and development?

stefancruz added a commit to stefancruz/Ocelot that referenced this pull request Dec 23, 2022

Merge branch 'develop@nurhat' into develop

cfbdb35

Cherry picked from ThreeMammals#1221

raman-m assigned nurhat Jul 19, 2023

raman-m changed the base branch from master to develop July 19, 2023 19:43

raman-m added the conflicts Feature branch has merge conflicts label Jul 19, 2023

raman-m force-pushed the develop branch from 2365273 to fa7f2af Compare July 28, 2023 17:24

raman-m changed the title ~~cache issues & ip filtering~~ Fix cache issue & IP filtering helpers Jul 28, 2023

raman-m added bug Identified as a potential bug proposal Proposal for a new functionality in Ocelot needs feedback Issue is waiting on feedback before acceptance and removed conflicts Feature branch has merge conflicts labels Jul 28, 2023

raman-m requested review from binarymash, jps, geffzhang, RaynaldM, philproctor, anktsrkr, sharpn, abooraja, wast, raman-m, yangzhongke and thiagoloureiro July 28, 2023 17:53

RaynaldM approved these changes Jul 31, 2023

View reviewed changes

wast reviewed Jul 31, 2023

View reviewed changes

src/Ocelot/Security/SecurityPolicyExtensions.cs Show resolved Hide resolved

nurhat and others added 7 commits October 12, 2023 17:45

add content language header for cache key generator

b4441c2

ip filtering try to find exact client i( proxy, firewall, loadbalance…

64ed0f3

… issues etc)

cache only if http status is OK

0cf7e01

schema can be modified by a service discovery provider

a2de83c

Fix errors. A small refactoring

604f223

Fix warnings

9892661

quick code review

9a1ad24

raman-m force-pushed the develop branch from d75eb08 to 9a1ad24 Compare October 12, 2023 16:17

raman-m removed request for binarymash, jps, geffzhang, philproctor, anktsrkr, sharpn, abooraja, raman-m, yangzhongke and thiagoloureiro November 10, 2024 14:53

raman-m added Security Options Ocelot feature: Security Options Headers Transform Ocelot feature: Headers Transformation conflicts Feature branch has merge conflicts and removed bug Identified as a potential bug needs feedback Issue is waiting on feedback before acceptance labels Nov 11, 2024

raman-m changed the title ~~Fix cache issue & IP filtering helpers~~ Follow up #1172 #1849: Fix cache issue. IP filtering helpers Nov 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow up #1172 #1849: Fix cache issue. IP filtering helpers #1221

Follow up #1172 #1849: Fix cache issue. IP filtering helpers #1221

nurhat commented May 5, 2020 •

edited by raman-m

Loading

EngRajabi commented May 24, 2020

garybond commented Feb 10, 2021

raman-m commented Jul 19, 2023

raman-m commented Jul 28, 2023 •

edited

Loading

raman-m commented Jul 28, 2023

raman-m commented Jul 28, 2023 •

edited

Loading

RaynaldM Jul 31, 2023

raman-m Aug 1, 2023

RaynaldM Aug 2, 2023

RaynaldM Jul 31, 2023

raman-m Aug 1, 2023

RaynaldM Aug 2, 2023

RaynaldM Jul 31, 2023

RaynaldM Jul 31, 2023

raman-m Aug 1, 2023 •

edited

Loading

RaynaldM Aug 2, 2023 •

edited

Loading

raman-m commented Jul 31, 2023 •

edited

Loading

wast Jul 31, 2023

raman-m commented Nov 11, 2024

raman-m commented Nov 11, 2024

	public static List<string> SplitCsv(this string csvList, bool nullOrWhitespaceInputReturnsNull = false)
	private static IReadOnlyCollection<string> SplitCsv(this string csvList, bool nullOrWhitespaceInputReturnsNull = false)

	return nullOrWhitespaceInputReturnsNull ? null : new List<string>();
	return nullOrWhitespaceInputReturnsNull ? null : Array.Empty<string>();

Follow up #1172 #1849: Fix cache issue. IP filtering helpers #1221

Are you sure you want to change the base?

Follow up #1172 #1849: Fix cache issue. IP filtering helpers #1221

Conversation

nurhat commented May 5, 2020 • edited by raman-m Loading

Proposed Changes

EngRajabi commented May 24, 2020

garybond commented Feb 10, 2021

raman-m commented Jul 19, 2023

raman-m commented Jul 28, 2023 • edited Loading

raman-m commented Jul 28, 2023

raman-m commented Jul 28, 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

raman-m Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

RaynaldM Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

raman-m commented Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

raman-m commented Nov 11, 2024

raman-m commented Nov 11, 2024

nurhat commented May 5, 2020 •

edited by raman-m

Loading

raman-m commented Jul 28, 2023 •

edited

Loading

raman-m commented Jul 28, 2023 •

edited

Loading

raman-m Aug 1, 2023 •

edited

Loading

RaynaldM Aug 2, 2023 •

edited

Loading

raman-m commented Jul 31, 2023 •

edited

Loading