Closed vantm closed 5 months ago
Hi Van! Thanks for PR submission! Welcome to Ocelot world! π―
Please, Be patient! I need some time to read and understand linked issues...
Extending route config by custom properties is interesting idea. Please keep in mind we have #1183 which is similar.
Extending route config by custom properties is interesting idea. Please keep in mind we have #1183 which is similar.
Are they really similar? The route metadata is a nice addition.
@ggnaegi commented on Dec 10
Right, the technical designs are different.. But these both issues are about the same custom properties for route. Similarity in this thing only. πββοΈ I've put the Accepted label onto the linked issue #738
@RaynaldM Thanks for the review! I have no idea which milestone we will include this PR in... Probable candidate for Jan'24 release if the author will be proactive...
All feedback has been resolved. Thank everybody for taking the time to work on my PR. Thank @RaynaldM for your reviews.
1st concern, Why Metadata
name?
We have other names from linked issues: Extensions
, see comment
Internal code of Ocelot middlewares uses the HttpContext
class Items
dictionary to keep extra data of the route/request, see HttpContext.Items Property
So, we use HttpContext.Items
dictionary across all middlewares...
2nd concern is IDictionary<string, string>
type of the Metadata
property. But developer will use another types like int
, double
, even object
... So, string
data type looks like a restriction in data types we can define in JSON...
Hi @raman-m
HttpContext.Items
here, it seems there is no restriction or issue of using them.string
is a restriction in data types While string
indeed restricts the data type, it doesn't restrict the data format. A string
can express many formats such as numbers, plain text, JSON, XML, HTML, etc.@vantm commented on Dec 13:
Why Use the Metadata Name? The linked issue lacks a detailed design; the term "Extensions" is merely a suggestion. In my opinion, "Metadata" carries a much broader meaning.
Yes, your suggestion has the priority. :smiley:
But could we make it configurable please?
For example, in global options we can propose a setting for "custom properties container" name.
By default, if the prop is not defined, it will be initialized by Metadata
value. Sounds good?
We use HttpContext.Items dictionary across all middlewares... As per my understands of Ocelot, it's a yes, but only when you want to access or manipulate configurations/states are stored here. Referencing these extension methods of
HttpContext.Items
here, it seems there is no restriction or issue of using them.
Yes, right. Developer can use these helpers always.
We need to recommend to developers this Items
approach of props propagation in the docs.
I believe it's not advisable to manage entities beyond our scope. That means there is no strong-typed binding, metadata validation, complex metadata structure handling, etc. It is simply attaching extra information to a route. That is why I ended up with the approach of using key/pair data type (this idea is similar to the k8s metadata annotations). Enabling support for a wide array of data types would necessitate heavy design and implementation overhead.
Yeap, parsing everything is not good. :rofl: But we can try to support at least numeric / integer types. Other types will remain as strings.
Using
string
is a restriction in data types Whilestring
indeed restricts the data type, it doesn't restrict the data format. Astring
can express many formats such as numbers, plain text, JSON, XML, HTML, etc.
Again, we need a basic support of integer, numeric, and string types.
@vantm Where are you?
@raman-m I found no reason to add support strong types in metadata. It only increases the complexity of the current codebase. IMO, key-value pairs in string provide almost all capabilities to developers to extend Ocelot.
@Burgyn Welcome to review!
Just a suggestion.
To me it would be nice to have an extension method directly above DownstreamRoute
.
Something like:
PreQueryStringBuilderMiddleware = async (context, next) =>
{
DownstreamRoute route = context.Items.DownstreamRoute();
var cachePolicies = route.GetMetadata<string[]>("CachePolicies");
...
}
@Burgyn IMO those kinds of extensions should be implemented by the developers because the formats of the metadata property vary (delimiter-separated values, JSON values, etc) and I want to keep our code base simple. But if we want to have those utils in the library, their signature could be:
static T GetJsonMetadataAsync<T>(this DownstreamRoute route, string key, JsonConvertOptions? options = null)
static string[] GetMetadataValuesAsync(
this DownstreamRoute route,
string key,
char separator = ',',
StringSplitOptions option = StringSplitOptions.RemoveEmptyEntries)
What are your thoughts?
@vantm commented on Mar 8
Hi @vantm, I understand and agree with the suggestion
@vantm commented on Mar 8
Hi Van!
Din't you add some extensions to the PR code already?
I looked into the Files changed and didn't find some extension methods marked by static
modifier keyword.
@raman-m Yes, I didn't work on it. I will implement those methods soon.
@vantm Tiny problem in integration tests.
@vantm Tiny problem in integration tests.
@raman-m Yes, it has been fixed, and extension methods are implemented as well. CC: @Burgyn
@vantm Thanks for working on it! Regarding https://github.com/ThreeMammals/Ocelot/pull/1843/commits/db3e994eec8583cb03bdd197723908e0f48f24a0 ... So, it makes sense to move Metadata from Configuration chapter because I expect that this chapter will be doubled in the size in coming future. And keeping Metadata docs as a section of Configuration chapter will make the chapter too long. π We need some cross-docs references and links as a small subsection for sure. I'll suggest them during code review... Let me focus on the current release Feb'24 now and I'll review after rebasing onto target and I'll invite the rest of the team to review...
I can push the changes for the author, but no feedback until now about the changes I proposed.
@raman-m @Burgyn the GetMetadataJsonSerializerOptions
, if you want to get a string[] you need to specify the separators etc. What I propose is to add some overridable global configuration settings with, as default parameters, separators = ',', trimChars = ' ' the ones used per default by @vantm. I think it should be enough. As for the JsonSerializerOptions
I would keep it as parameter of the GetMetadata
@raman-m @vantm I just pushed a first version of a generic GetMetadata
@ggnaegi Very well!
However, where is the generic method definition, specifically the Method<T>()
syntax?
I would expect something like this:
- private static object ConvertTo(Type targetType, string value, MetadataOptions metadataOptions, JsonSerializerOptions jsonSerializerOptions)
+ private static T ConvertTo<T>(string value, MetadataOptions metadataOptions, JsonSerializerOptions jsonSerializerOptions)
This format is more intuitive for generic definitions. However, retaining the old version that returns an object
is also acceptable and recommended.
@raman-m What's wrong here? I rebased against Release/24.0 ... Suddenly I have quite a lot of files modified here.
@ggnaegi Could you please explain why you rebased this feature branch again? It appears the branch had already been rebased onto release/24. Why was this done?
Keep in mind that if rebasing occurs on the origin, you'll need to remove the branch in your local repository, fetch all changes from the origin, and then check out the branch again. If your local branch differs from the origin, you need not to force a rebase: just re-checkout!
It seems that the local release/24.0
was not synchronized... as there are now commits from develop. It appears you have rebased onto develop, which is incorrect.
Parallel releases are currently in progress. The develop
branch and the release/24.0
branch must not be synchronized or merged with one another!
Seems "March-April'24" will be released first. After that we will rebase onto, or just merge develop
into release/24.0
.
@ggnaegi requested review from @raman-m, @RaynaldM and @ggnaegi on May 3
I'm unable to review due to redundant commits from the develop branch. Please create a backup branch to preserve the current state locally, then rebase onto ThreeMammals:release/24.0. If you require assistance, feel free to ask; I have a reliable local copy of the feature branch and can perform another force push if necessary.
@ggnaegi requested review from @raman-m, @RaynaldM and @ggnaegi on May 3
I'm unable to review due to redundant commits from the develop branch. Please create a backup branch to preserve the current state locally, then rebase onto ThreeMammals:release/24.0. If you require assistance, feel free to ask; I have a reliable local copy of the feature branch and can perform another force push if necessary.
@raman-m Ok, then just do that, I don't have a reliable local copy anymore... Thanks. I probably forgot to delete the local copy and checkout.
@raman-m all my commits are gone? π
@ggnaegi Negative! Your commits are preserved! The feature branch has been successfully rebased onto ThreeMammals:release/24.0β Done! There are no redundant commits from develop.
If you want to contribute more, just delete branch locally, fetch all and checkout once again.
@ggnaegi approved these changes on May 3
I apologize, but where can I find your approval?
I will conduct a review today as well, since it appears that most of the delivery artifacts, including documentation, are present.
@raman-m , the documentation might need to be completed though...
Excellent work, team, @ggnaegi @vantm πͺ Here are my suggestions:
Major Issues:
- The feature functionality should be organized by software features (modules): in Dependency Injection, separate folders.
- The use of .NET code reflection technology in the library is fundamentally incorrect!
Sorry! We can't merge it for now! Let's work a bit more...
Why is code reflection a problem? Performance, maintainability?. I used the code reflection to avoid issues with functionalities that aren't supported in .NET 6 yet.
I don't agree with you, until we provide coding standards, this PR is good to be merged.
@ggnaegi Gui,
Why is code reflection a problem? Performance, maintainability ?.
Code reflection can pose issues with performance and seems no issues with maintainability. Indeed, you've got it right! In the context of Ocelot, if reflection occurs only at application startup, it typically doesn't present a problem. However, if reflection is executed for each request, it can lead to performance degradation by adding extra milliseconds to the request's processing time. Finally, I'd say: reflection is a code smell, for high load systems it is real performance problem.
I used the code reflection to avoid issues with functionalities that aren't supported in .NET 6 yet.
Which functionality is unavailable in .NET 6? Is it the Parse
method of a numeric type? π You said: the exact issue addressed with the reflected "Parse" method was to bypass the absence of certain functionalities. But which functionality?
Why not invoke Parse
directly?
Are you familiar with the IConvertible
interface?
I don't agree with you, until we provide coding standards, this PR is good to be merged.
I concur that merging is a good step, but it's better to address the issues I've identified. If you're short on time, I can tackle the problems highlighted in my code review. However, my immediate focus is on the current monthly release. Once that's handled, I'll return to this pull request.
I concur that merging is a good step, but it's better to address the issues I've identified. If you're short on time, I can tackle the problems highlighted in my code review. However, my immediate focus is on the current monthly release. Once that's handled, I'll return to this pull request.
@raman-m what monthly release? It wasn't planed like that!
@ggnaegi
what monthly release? It wasn't planed like that!
This monthly release → March-April'24 And what we do now → Milestones
@raman-m ready for re-review
@ggnaegi Hey, mentor! What about pressing magic Squash button? πͺ
@raman-m in release/24.0, really? We can still revert it, but I'm not sure about the logic now...
ThreeMammals:release/24.0 will be rebased onto develop soon, after current milestone release. @ggnaegi What's your concern? Why to revert? Do you want to merge everything to develop, into the current release? We can discuss the process, seems you like GitHub workflow. We have 2 parallel releases following Git workflow. But our work is too slow. Make sense to switch to GitHub workflow. What I propose now is
Sounds like a plan?
The issue lies within the use of Milestones... I attempt to structure and schedule releases by employing milestones. If we transition to a GitHub workflow (consolidating all changes into the develop branch) and adopt a monthly release cycle, then planning with Milestones seems redundant, rendering them essentially artificial. Currently, I organize each release within a distinct milestone collection, allowing each release to have its own milestone. This is a feature unique to the Git workflow, which facilitates the development of releases in parallel. I have discovered that Milestones are beneficial and wish to continue utilizing them. Another argument, milestone has own version, which is good. GitHub workflow makes a zoo of versions. I don't like this process personally.
@ggnaegi, rest assured, this feature is slated for release in version 23.3. Although the branch is incorrect, cherry-picking the feature to the develop branch should not pose a technical issue.
@raman-m ok, fine, it's great!
Feature commit in develop → https://github.com/ThreeMammals/Ocelot/commit/573a9d98e3eb730a53a555d881df15de9017b43d
Closes #738 #1990
738
1990
Related to
651
Proposed Changes
Route metadata allows Ocelot users to add custom functions that solve ad-hoc requirements or maybe write their plugins/extensions.
The new configuration will look like:
Now, the metadata is available in the
DownstreamRoute
object