#651 Merge custom JSON properties across multiple `ocelot.X.json` configuration files using Newtonsoft's `JToken` merge functionality #1183

jlukawska · 2020-04-07T19:45:35Z

Closes #651

Extending Route configuration #651

Proposed Changes

The whole sections of config files (with custom properties) are merged.

raman-m · 2023-07-26T15:18:32Z

Congrats, @jlukawska ! 🎉
Your feature branch has been rebased onto ThreeMammals:develop.

We can go with code review now...

raman-m · 2023-08-03T18:24:03Z

@nurhat
As the #651 issue author, could you review this solution please?
Will the solution be useful and helpful for your daily gateway development?

raman-m · 2023-08-03T18:30:05Z

@RaynaldM @wast
You are great approvers! 🥇
Seems like @jlukawska is making breaking changes and you've approved them. 🤣
I am not sure about changes that they're breaking on 100%, but I will review the code thoroughly. 👇

raman-m

@jlukawska
Could you fix the issues please?

How to read CustomStrategyProperty value of the route in run-time?

raman-m · 2023-08-03T18:41:09Z

src/Ocelot/DependencyInjection/ConfigurationBuilderExtensions.cs

-
-                fileConfiguration.Aggregates.AddRange(config.Aggregates);
-                fileConfiguration.Routes.AddRange(config.Routes);
+                MergeConfig(fileConfiguration, config, isGlobal);                


🆗 Merged config JSON goes to primaryConfigFile. And it will be serialized to ocelot.json file.
And the primary config will be attached to IConfigurationBuilder object by AddJsonFile method.

Could you show me a case how can I read this custom property inside of extension-methods for IConfigurationBuilder interface?

test/Ocelot.AcceptanceTests/ConfigurationMergeTests.cs

raman-m · 2023-08-03T19:27:52Z

@nurhat @jlukawska
I would like to understand, how useful this feature for real world user scenarios.

User case 1

Change behavior of PollyQoSProvider

Given, I wish to override behavior of the Quality of Service feature by changes in PollyQoSProvider and/or AddPolly and/or PollyCircuitBreakingDelegatingHandler classes.
And, I will define custom property of the route for custom QoS overriding in a ocelot.xservices.json file.
And, I will read the custom property from DownstreamRoute object after merging all configs.
When, I read the custom property from DownstreamRoute object.
Then, I get stuck! 😉

Question:

How to read custom property from DownstreamRoute object?
How and what interface do I need to inject to a service class to be able to read custom property from DownstreamRoute object?

jlukawska · 2024-05-16T17:47:47Z

@raman-m, I marked this PR as a draft, because I am not sure if the conflicts are resolved correctly (even though the tests passed correctly). It wasn't that easy ;) I'd have to revise the code, understand the changes from develop branch and maybe the PR code would have to be improved.
Apart from that, would this PR be helpful for anyone? I proposed it 4 years ago and the issue reporter no longer responds.

raman-m · 2024-05-28T09:05:08Z

Understood, Jolanta. It seems you're uncertain and wish to further develop. I also notice that many PR files contain a fake diff indicating numerous merge conflicts were resolved.

Here's what I suggest:

Create a new feature branch from develop. Your forked repo develop is synced.
Clone the second repository for manual file operations.
Transfer all changes from the old feature branch to the new one using a file manager (Total Commander is my choice, an old school tool with a dual-pane interface vs useful comparison command).
Open a new PR after confirming successful compilation and passing tests.
Close this old PR and reference the new one.

Does this sound like a good approach?

P.S.
I believe the most important changes are in the file: src/Ocelot/DependencyInjection/ConfigurationBuilderExtensions.cs
which should be double checked.

raman-m · 2024-11-11T12:57:29Z

🆗 I'm going to rebase the branch...

potential warnings

raman-m · 2024-11-11T16:17:27Z

@jlukawska commented on May 16, 2024

I am not sure if the conflicts are resolved correctly (even though the tests passed correctly). It wasn't that easy ;)

I've rebased the feature branch today and addressed the failed tests. The entire code appears to be in good shape. I will provide my final code review later, after receiving feedback from the team. However, the current acceptance test confirms the successful merging of JSON. More tests are necessary, and it seems they will be essential for the upcoming C# helpers that will be reused in the actual code of middlewares, similar to how the Metadata-helpers function.

Apart from that, would this PR be helpful for anyone?

Indeed, the question of whether this PR would be beneficial to anyone is quite pertinent, especially after the introduction of the Route Metadata feature (aka Metadata). While some developers may not favor storing additional route data in the Metadata property, the direct inclusion of properties in JSON remains a sought-after feature, as suggested by your PR. The primary challenge lies in crafting comprehensive documentation before we can develop truly useful C# helpers, potentially surpassing the current Metadata logic.

I proposed it 4 years ago and the issue reporter no longer responds.

Mr. @nurhat didn't even reply to me, which suggests he might have used a work account linked to a corporate email. Developers often change jobs and employers, along with their work emails. Regardless, I'll assume the role of the author. 😉

raman-m

Ready for Code Review❕

raman-m · 2024-11-11T16:40:40Z

Successors

Remove the Newtonsoft.Json package and migrate to System.Text.Json #2125

This PR must be merged first❗

raman-m assigned jlukawska Jul 20, 2023

raman-m changed the title ~~Feature/651 merge custom properties of config file~~ #651 Merge custom properties of config file Jul 20, 2023

raman-m changed the base branch from master to develop July 20, 2023 15:24

raman-m self-requested a review July 20, 2023 16:05

raman-m added the feature A new feature label Jul 20, 2023

raman-m force-pushed the feature/651-merge-custom-properties branch from 4eb4482 to 9ec4b8d Compare July 26, 2023 14:36

raman-m requested review from binarymash, jps, geffzhang, RaynaldM, philproctor, anktsrkr, sharpn, abooraja, wast, yangzhongke, margaale and thiagoloureiro July 26, 2023 15:19

raman-m mentioned this pull request Jul 26, 2023

Extending Route configuration #651

Open

raman-m added the needs feedback Issue is waiting on feedback before acceptance label Jul 26, 2023

RaynaldM approved these changes Jul 28, 2023

View reviewed changes

raman-m mentioned this pull request Aug 1, 2023

Use merged ocelot.json directly from memory instead of from file #1216

Closed

wast approved these changes Aug 2, 2023

View reviewed changes

raman-m requested changes Aug 3, 2023

View reviewed changes

raman-m mentioned this pull request Aug 18, 2023

#692 Simplify downstream service config #1353

Closed

raman-m force-pushed the feature/651-merge-custom-properties branch from 04debe6 to 7f61d01 Compare October 12, 2023 12:16

raman-m added Configuration Ocelot feature: Configuration and removed needs feedback Issue is waiting on feedback before acceptance labels Nov 11, 2024

jlukawska and others added 16 commits November 11, 2024 16:15

ThreeMammals#651 merge custom properties

8d9f9e4

Jobject + acceptance test

d3b8d8f

ThreeMammals#651 acceptance test, small changes

0f355af

Update ConfigurationBuilderExtensions.cs

c72b8f4

potential warnings

Fix compilation errors

059842f

Fix compilation errors

a43c8de

Fix test: Should_merge_routes_custom_properties

6d1ac97

AddOcelot overloaded version

adbfb10

Rename test class

8d84fd6

Refactor Then methods

2f33574

IFileConfigurationRepository : Rework Get method

8679fda

Add GetMessages helper

b1ca7f5

IFileConfigurationRepository : Rework Set method

60dcf58

IFileConfigurationRepository: Rename to SetAsync

67b7e5b

merge fix

4eedbae

Fix build after rebasing, with a refactoring

1b2198c

raman-m force-pushed the feature/651-merge-custom-properties branch from 8c3ec5a to 1b2198c Compare November 11, 2024 15:47

raman-m reviewed Nov 11, 2024

View reviewed changes

raman-m requested review from raman-m, RaynaldM and ggnaegi November 11, 2024 16:22

raman-m changed the title ~~#651 Merge custom properties of config file~~ #651 Merge custom JSON properties across multiple ocelot.X.json configuration files using Newtonsoft's JToken merge functionality Nov 11, 2024

raman-m mentioned this pull request Nov 11, 2024

Remove the Newtonsoft.Json package and migrate to System.Text.Json #2125

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#651 Merge custom JSON properties across multiple `ocelot.X.json` configuration files using Newtonsoft's `JToken` merge functionality #1183

#651 Merge custom JSON properties across multiple `ocelot.X.json` configuration files using Newtonsoft's `JToken` merge functionality #1183

jlukawska commented Apr 7, 2020 •

edited by raman-m

Loading

raman-m commented Jul 26, 2023

raman-m commented Aug 3, 2023

raman-m commented Aug 3, 2023 •

edited

Loading

raman-m left a comment •

edited

Loading

raman-m Aug 3, 2023 •

edited

Loading

raman-m commented Aug 3, 2023 •

edited

Loading

jlukawska commented May 16, 2024

raman-m commented May 28, 2024 •

edited

Loading

raman-m commented Nov 11, 2024

raman-m commented Nov 11, 2024

raman-m left a comment

raman-m commented Nov 11, 2024 •

edited

Loading

#651 Merge custom JSON properties across multiple ocelot.X.json configuration files using Newtonsoft's JToken merge functionality #1183

Are you sure you want to change the base?

#651 Merge custom JSON properties across multiple ocelot.X.json configuration files using Newtonsoft's JToken merge functionality #1183

Conversation

jlukawska commented Apr 7, 2020 • edited by raman-m Loading

Closes #651

Proposed Changes

raman-m commented Jul 26, 2023

raman-m commented Aug 3, 2023

raman-m commented Aug 3, 2023 • edited Loading

raman-m left a comment • edited Loading

Choose a reason for hiding this comment

raman-m Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

raman-m commented Aug 3, 2023 • edited Loading

User case 1

Change behavior of PollyQoSProvider

Question:

jlukawska commented May 16, 2024

raman-m commented May 28, 2024 • edited Loading

raman-m commented Nov 11, 2024

raman-m commented Nov 11, 2024

raman-m left a comment

Choose a reason for hiding this comment

Ready for Code Review❕

raman-m commented Nov 11, 2024 • edited Loading

Successors

#651 Merge custom JSON properties across multiple `ocelot.X.json` configuration files using Newtonsoft's `JToken` merge functionality #1183

#651 Merge custom JSON properties across multiple `ocelot.X.json` configuration files using Newtonsoft's `JToken` merge functionality #1183

jlukawska commented Apr 7, 2020 •

edited by raman-m

Loading

raman-m commented Aug 3, 2023 •

edited

Loading

raman-m left a comment •

edited

Loading

raman-m Aug 3, 2023 •

edited

Loading

raman-m commented Aug 3, 2023 •

edited

Loading

raman-m commented May 28, 2024 •

edited

Loading

raman-m commented Nov 11, 2024 •

edited

Loading