[FEATURE REQ] - Export Policy as XML instead of RAWXML

[Banchio]

Please describe the feature.

Related to #488
Re-posting this as the new version goes toward definitive release. When exporting policies, the rawxml format is not valid for other programmatic scenario (think of select-xml cmdlet in powershell).
Proposal is to add a new parameter POLICY_SPECIFICATION_FORMAT with two values: rawxml, default one as of today and xml which export policies in the escaped format.
This setting and behavior was proposed in this PR #489 , I can try to reimplement that login in the new version but do not want to waste time if this get refused again. Thanks

[github-actions]

  Thank you for opening this issue! Please be patient while we will look into it and get back to you as this is an open source project. In the meantime make sure you take a look at the [closed issues](https://github.com/Azure/apiops/issues?q=is%3Aissue+is%3Aclosed) in case your question has already been answered. Don't forget to provide any additional information if needed (e.g. scrubbed logs, detailed feature requests,etc.).
  Whenever it's feasible, please don't hesitate to send a Pull Request (PR) our way. We'd greatly appreciate it, and we'll gladly assess and incorporate your changes.

[guythetechie]

Thanks, @Banchio. The proposal makes sense.

Please wait until our next RC release with support for workspaces though (should be sometime next week). Don't want you to make changes while the code-base shifts under you.

[waelkdouh]

@Banchio I added your proposal as a task on the apiops board. Once we release v6 rc2 you can start implementing the feature and submit a pr so we can review and merge. Please let me know when you start implementing so I can move the task to in progress status. Thanks in advance for your contributions.

[guythetechie]

@Banchio - code has been merged to main, please feel free to start working on this. The code base has changed significantly since your last PR, so don't hesitate to ask any questions.

[Banchio]

thanks @guythetechie, may I ask you to have a look at this commit on my fork? I basically expect a new parameter and pass it wherever there's a policy export. Do you think we have impact on publisher as well?

[guythetechie]

Looks great, thanks!

The publisher will be impacted as well; it expects a format in the policy PUT request. We currently hardcode it here for example:

apiops/tools/code/publisher/ApiPolicy.cs

Line 148 in bf5cc12

Format = "rawxml",

Since it's needed in both the extractor and publisher, your PolicySpecification.cs file can live in the common project. This also allows more expressive parameter types, from this:

GetDto(this ProductPolicyUri uri, HttpPipeline pipeline, CancellationToken cancellationToken, string policyFormat)

to this:

GetDto(this ProductPolicyUri uri, HttpPipeline pipeline, CancellationToken cancellationToken, DefaultPolicySpecification policyFormat)

One last thing: the REST API calls this PolicyContentFormat, so I would call the type that or PolicyFormat. You may want to consider two separate types:

• PolicyContentFormat or PolicyFormat. This will be used in the GetDto policy methods in the common project. Values can be one of these.
  

• DefaultPolicyContentFormat or DefaultPolicyFormat. This will be specified via configuration in the extractor or publisher. Values can be one of PolicyFormat.Xml or PolicyFormat.RawXml. You don't have to create this type, but then you'd be passing around PolicyFormat in your extractor and publisher DI where the value could be incorrectly set to xml-link or something else. I think having a separate type for the default makes your intent clear, and you can limit it to only allowed values.

[Banchio]

thank you @guythetechie very detailed. I tried to implement what you said. Specifically renamed to Policy Content Format and moved to common. I did not understand the DefaultPolicyContentFormat object in the extractor, I parsed the parameter and inject in the DI the object defined in the common have a look here.
I'll wait for a feedback and if okay work on the publisher, I want to complete this before having some time off at the end of the week. Thanks again!

Branch: xml-fromconfig branch

[guythetechie]

@Banchio - What you have looks great. Here is what I was suggesting, but it's not necessary.

For API specifications, we have an ApiSpecification type that has several options. We use it whenever the code requires a specification.

apiops/tools/code/common/ApiSpecification.cs

Line 12 in bf5cc12

public abstract record ApiSpecification

In the extractor though, we also have a DefaultApiSpecification type. We use it whenever the code is explicitly looking for the default API specification passed in configuration. Think of it as a more specific version of the ApiSpecification type. Though not implemented, we could have restricted allowed values only to the ones supported in configuration.

apiops/tools/code/extractor/ApiSpecification.cs

Line 11 in bf5cc12

internal sealed record DefaultApiSpecification(ApiSpecification Value);

The advantage of also having a DefaultApiSpecification type is that we can safely pass it in DI, and everybody knows it's referring to the default API specification. If instead we pass a singleton ApiSpecification type in DI, it's less clear. Are we referring to the default API specification? Or are we referring to another use case for API specifications?

Right now, the only use case for passing API specifications in DI is to refer to the default value passed in configuration. We could get away just using ApiSpecification. If another use case came in though, we'd have to introduce a way of distinguishing the default configuration ApiSpecification in DI with the other ApiSpecification for that use case.

Hope this makes sense. I can't think of any other reason why we would want to pass the policy content format in DI, which is why I think your implementation is fine. Adding a second DefaultPolicyContentFormat type would make things a bit clearer though, and it would help future-proof the implementation. It could also be unnecessary over-engineering... :)

I'll let you make that choice. If you decide not to implement a DefaultPolicyContentFormat type, that's perfectly fine. We can implement it later if we ever need to make a distinction.