Skip to content

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

Open
nurhat wants to merge 5 commits intoThreeMammals:developfrom
nurhat:develop
Open

Follow up #1172 #1849: Fix cache issue. IP filtering helpers#1221
nurhat wants to merge 5 commits intoThreeMammals:developfrom
nurhat:develop

Conversation

@nurhat
Copy link

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

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
Copy link

EngRajabi commented May 24, 2020

#1172

@garybond
Copy link

garybond commented Feb 10, 2021

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

stefancruz added a commit to stefancruz/Ocelot that referenced this pull request Dec 23, 2022
@stefancruz
Merge branch 'develop@nurhat' into develop
cfbdb35 
Cherry picked from ThreeMammals#1221
@raman-m raman-m changed the base branch from master to develop July 19, 2023 19:43
@raman-m raman-m added the conflicts Feature branch has merge conflicts label Jul 19, 2023
@raman-m
Copy link
Member

raman-m commented Jul 19, 2023

@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 raman-m changed the title cache issues & ip filtering Fix cache issue & IP filtering helpers Jul 28, 2023
@raman-m 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
Copy link
Member

raman-m commented Jul 28, 2023 •
edited
Loading

@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
Copy link
Member

raman-m commented Jul 28, 2023

@EngRajabi commented on May 24, 2020

Mohsen, could you review the code please?

@raman-m
Copy link
Member

raman-m commented Jul 28, 2023 •
edited
Loading

@garybond commented on Feb 10, 2021:

Any update on when we might get this?

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

RaynaldM
RaynaldM approved these changes Jul 31, 2023
View reviewed changes
src/Ocelot/Security/SecurityPolicyExtensions.cs
return string.Empty;
}

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

@RaynaldM RaynaldM Jul 31, 2023

Choose a reason for hiding this comment

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

could be private

Copy link
Member

@raman-m raman-m Aug 1, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@RaynaldM RaynaldM Aug 2, 2023

Choose a reason for hiding this comment

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

unless no one uses it outside of this class

src/Ocelot/Security/SecurityPolicyExtensions.cs
return string.Empty;
}

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

@RaynaldM RaynaldM Jul 31, 2023

Choose a reason for hiding this comment

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

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

Copy link
Member

@raman-m raman-m Aug 1, 2023

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@RaynaldM RaynaldM Aug 2, 2023

Choose a reason for hiding this comment

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

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

src/Ocelot/Security/SecurityPolicyExtensions.cs
{
if (string.IsNullOrWhiteSpace(csvList))
{
return nullOrWhitespaceInputReturnsNull ? null : new List<string>();
Copy link
Contributor

@RaynaldM RaynaldM Jul 31, 2023

Choose a reason for hiding this comment

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

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

src/Ocelot/Security/SecurityPolicyExtensions.cs
.Split(',')
.AsEnumerable()
.Select(s => s.Trim())
.ToList();
Copy link
Contributor

@RaynaldM RaynaldM Jul 31, 2023

Choose a reason for hiding this comment

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

Suggested change
.ToList();
.ToArray();

Copy link
Member

@raman-m raman-m Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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. 🤣

Copy link
Contributor

@RaynaldM RaynaldM Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

@raman-m
Copy link
Member

raman-m commented Jul 31, 2023 •
edited
Loading

@RaynaldM Ray,
Thanks for your code review! 🤝

@raman-m raman-m force-pushed the develop branch 7 times, most recently from c216cdf to b458f0f Compare April 7, 2025 16:13
@raman-m raman-m force-pushed the develop branch 17 times, most recently from 2e07528 to 33e9c23 Compare April 18, 2025 17:50
@coveralls
Copy link
Collaborator

coveralls commented Jan 14, 2026

Coverage Status

coverage: 93.354% (-0.1%) from 93.503%
when pulling c5a559b on nurhat:develop
into 9fc4e78 on ThreeMammals:develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@raman-m raman-m raman-m left review comments

+2 more reviewers

@RaynaldM RaynaldM RaynaldM approved these changes

@wast wast wast left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@nurhat nurhat

Labels

Headers Transform Ocelot feature: Headers Transformation proposal Proposal for a new functionality in Ocelot Security Options Ocelot feature: Security Options

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@nurhat @EngRajabi @garybond @raman-m @coveralls @RaynaldM @wast