Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#738 #1990 Route Metadata as custom properties #1843

Merged
merged 32 commits into from
May 21, 2024

Conversation

vantm
Copy link
Contributor

@vantm vantm commented Dec 7, 2023

Closes #738 #1990

Related to

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
}

@vantm vantm force-pushed the feat/route-metadata branch 2 times, most recently from a97bb42 to b3094b2 Compare December 7, 2023 16:49
@vantm vantm marked this pull request as ready for review December 7, 2023 16:53
@raman-m
Copy link
Member

raman-m commented Dec 8, 2023

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 raman-m added the proposal Proposal for a new functionality in Ocelot label Dec 8, 2023
@raman-m
Copy link
Member

raman-m commented Dec 8, 2023

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

@ggnaegi
Copy link
Member

ggnaegi commented Dec 10, 2023

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.

src/Ocelot/Configuration/Builder/DownstreamRouteBuilder.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/Creator/IMetadataCreator.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/Creator/MetadataCreator.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/Creator/MetadataCreator.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/Creator/MetadataCreator.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/DownstreamRoute.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/DownstreamRoute.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/File/FileDynamicRoute.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/File/FileGlobalConfiguration.cs Outdated Show resolved Hide resolved
src/Ocelot/Configuration/File/FileRoute.cs Outdated Show resolved Hide resolved
@raman-m raman-m changed the title Feature: Route Metadata #738 Route Metadata as custom properties Dec 11, 2023
@raman-m
Copy link
Member

raman-m commented Dec 11, 2023

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

@raman-m raman-m added feature A new feature and removed proposal Proposal for a new functionality in Ocelot labels Dec 11, 2023
@vantm
Copy link
Contributor Author

vantm commented Dec 11, 2023

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

@raman-m
Copy link
Member

raman-m commented Dec 12, 2023

  • 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
Copy link
Contributor Author

vantm commented Dec 13, 2023

Hi @raman-m

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

@ggnaegi
Copy link
Member

ggnaegi commented May 17, 2024

@raman-m ready for re-review

@raman-m raman-m assigned vantm and ggnaegi and unassigned vantm and ggnaegi May 21, 2024
@raman-m
Copy link
Member

raman-m commented May 21, 2024

Ready for delivery ✅

  • Code review ✔️✔️✔️
  • Team approvals ✔️✔️✔️
  • Unit testing ✔️✔️ → DefaultMetadataCreatorTests (link1) and DownstreamRouteExtensionsTests (link2)
  • Acceptance testing ✔️ → DownstreamMetadataTests (link)
  • Updated feature docs ✔️ → metadata.rstlink

@raman-m
Copy link
Member

raman-m commented May 21, 2024

@ggnaegi Hey, mentor!
What about pressing magic Squash button? 🪄

@ggnaegi ggnaegi merged commit 4f7b46d into ThreeMammals:release/24.0 May 21, 2024
1 check passed
@ggnaegi
Copy link
Member

ggnaegi commented May 21, 2024

@raman-m in release/24.0, really? We can still revert it, but I'm not sure about the logic now...

@raman-m
Copy link
Member

raman-m commented May 21, 2024

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?

@raman-m
Copy link
Member

raman-m commented May 21, 2024

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.

@raman-m raman-m added Spring'24 Spring 2024 release and removed 2023 Annual 2023 release labels May 22, 2024
@raman-m raman-m modified the milestones: Annual 2023, Spring'24 May 22, 2024
@raman-m
Copy link
Member

raman-m commented May 22, 2024

The release label has been updated to Spring'24

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

@ggnaegi
Copy link
Member

ggnaegi commented May 22, 2024

@raman-m ok, fine, it's great!

raman-m added a commit that referenced this pull request May 23, 2024
* feat(configuration): adding route metadata

* feat(configuration): update docs

* feat(configuration): replace Dictionary<> by IDictionary<>, code cleaning

* feat(configuration): replace Dictionary<> by IDictionary<>

* feat(configuration): replace Dictionary<> by IDictionary<>

* feat(configuration): update the data type of FileDynamicRoute Metadata

* formatting

* feat(configuration): fix integration tests

* feat !1843 add extension methods for DownstreamRoute to get metadata

* feat !1843 add extension methods for DownstreamRoute

* feat !1843 update docs

* feat !1843 update docs

* feat !1843 cleanup split string logic

* SA1505: An opening brace should not be followed by a blank line

* IDE1006: Naming rule violation: These words must begin with upper case characters: should_xxx

* Fix compile errors after rebasing

* Fix unit tests + AAA pattern

* First Version, providing a generic extension method GetMetadata<T> with global configuration

* Adding ConvertToNumericType method to be able to use the NumberStyles enum

* adding first acceptance tests

* The tests are now passing again...

* adding latest test cases. That should be enough (includes global configuration changes too)

* Update metadata.rst

* adding the xml docs for IMetadataCreator and MetadataCreator

* renaming MetadataCreator to DefaultMetadataCreator

* number tests for .net 6 too

* Moving Metadata specific downstream route extensions to the Metadata folder

* cleanup

* applying some of the requested changes

* Final code review by @raman-m

* Add traits

* Fix docs build error

---------

Co-authored-by: Raman Maksimchuk <[email protected]>
Co-authored-by: Guillaume Gnaegi <[email protected]>
@raman-m
Copy link
Member

raman-m commented May 23, 2024

Feature commit in develop → 573a9d9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Configuration Ocelot feature: Configuration feature A new feature Routing Ocelot feature: Routing Spring'24 Spring 2024 release
Projects
None yet
5 participants