#738 #1990 Route Metadata as custom properties

[vantm]

Closes #738 #1990

• #738
• #1990

Related to

• #651

Proposed Changes

• Adding metadata to the global configuration
• Adding metadata to the route configuration

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:

{
  "Routes": [
      {
          "UpstreamHttpMethod": [ "GET" ],
          "UpstreamPathTemplate": "/posts/{postId}",
          "DownstreamPathTemplate": "/api/posts/{postId}",
          "DownstreamHostAndPorts": [
              { "Host": "localhost", "Port": 80 }
          ],
          "Metadata": {
              "api-id": "FindPost",
              "my-extension/param1": "overwritten-value",
              "other-extension/param1": "value1",
              "other-extension/param2": "value2",
              "tags": "tag1, tag2, area1, area2, feat1"
          }
      }
  ],
  "GlobalConfiguration": {
      "Metadata": {
          "server_name": "data-center-1-machine-1",
          "my-extension/param1": "default-value"
      }
  }
}

Now, the metadata is available in the DownstreamRoute object

var downstreamRoute = context.Items.DownstreamRoute();

if(downstreamRoute?.Metadata is {} metadata)
{
    var param1 = metadata.GetValueOrDefault("my-extension/param1") ?? throw new MyExtensionException("Param 1 is null");
    var param2 = metadata.GetValueOrDefault("my-extension/param2", "custom-value");

    // working with metadata
}

[raman-m]

Hi Van! Thanks for PR submission! Welcome to Ocelot world! 🐯

Please, Be patient! I need some time to read and understand linked issues...

[raman-m]

Extending route config by custom properties is interesting idea. Please keep in mind we have #1183 which is similar.

[ggnaegi]

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.

[raman-m]

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

[vantm]

All feedback has been resolved. Thank everybody for taking the time to work on my PR. Thank @RaynaldM for your reviews.

[raman-m]

• 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...

[vantm]

Hi @raman-m

1. • 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.
   • 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.
2. • 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.
   • Using 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.

[raman-m]

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

Again, we need a basic support of integer, numeric, and string types.

[raman-m]

@vantm Where are you?

[vantm]

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

[raman-m]

@Burgyn Welcome to review!

[Burgyn]

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");
   ...
}

[vantm]

@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?

[Burgyn]

@vantm commented on Mar 8

Hi @vantm, I understand and agree with the suggestion

[raman-m]

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

[vantm]

@raman-m Yes, I didn't work on it. I will implement those methods soon.

[raman-m]

@vantm Tiny problem in integration tests.

[vantm]

@vantm Tiny problem in integration tests.

@raman-m Yes, it has been fixed, and extension methods are implemented as well. CC: @Burgyn

[raman-m]

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

[ggnaegi]

I can push the changes for the author, but no feedback until now about the changes I proposed.

[raman-m]

@vantm Dear Van, could you please begin addressing the issues from both of our reviews?

• Code review by @ggnaegi on Apr 3 here
• My code review on Apr 5 here

Additionally, we need to write acceptance tests. 👇

PRs lacking acceptance tests for new features cannot be merged❕

[ggnaegi]

@raman-m @Burgyn the GetMetadata<T> approach is feasible, but as mentioned by @vantm we will have several parameters that could be modified: If your string is a json, you should pass JsonSerializerOptions, 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<T> method with default value = null, and if null use Default.Web.

[ggnaegi]

@raman-m @vantm I just pushed a first version of a generic GetMetadata<T> extension method

[raman-m]

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

[ggnaegi]

@raman-m What's wrong here? I rebased against Release/24.0 ... Suddenly I have quite a lot of files modified here.

[raman-m]

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

⚠️ Important Notice

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.

[raman-m]

@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]

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

[ggnaegi]

@raman-m all my commits are gone? 😅

[raman-m]

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