Reshuffle shared source files

[annelo-msft]

As pre-work to address:

• Support SDK generation outside Azure/azure-sdk-for-net #38143
• ClientModel: Migrate stable types from Azure.Core Shared source to public types #28369
• Shared source consumed by both Azure.Core and autorest.csharp creates a circular dependency #27337

I'm working on an accounting of our shared source files and which libraries use them.

To make this easier, I'd like to at least temporarily move files used by only a single library into that library. If any of these files need to stay shared, please let me know because that will be useful information in our prioritization and planning around the issues listed above.

This PR specifically:

• Moves GZipUtf8JsonRequestContent to Monitor
• Moves GuidUtilities to ServiceBus
• Makes GeoRedundantFallbackPolicy internal
• Moves ErrorResponse<T> into HttpPipelineExtensions
• Moves NoValueResponse<T> to Tables

It's worth noting that the last two types address the same API need, and if we make these public as part of #28369, we should likely merge them into a single type.

[azure-sdk]

API change check

API changes are not detected in this pull request.

[christothes]

I think we should keep NoValueResponse<T> in Core Shared, as it should be the established pattern for returning a response with no value. I think moving it potentially limits discoverability for future clients. Storage is one example I am aware of that should be able to utilize it.

[annelo-msft]

I think we should keep NoValueResponse in Core Shared, as it should be the established pattern for returning a response with no value. I think moving it potentially limits discoverability for future clients. Storage is one example I am aware of that should be able to utilize it.

How would you feel about moving it temporarily and then driving consistency as a holistic effort when we move it back to being a shared resource?

One observation here is that we've created two of these types that do the same thing with NoValueResponse<T> and ErrorResponse<T>. It would be good to consolidate and merge these as part of this effort. Until we do this, I feel strange about having our libraries take a dependency on one or the other.

[christothes]

How would you feel about moving it temporarily and then driving consistency as a holistic effort when we move it back to being a shared resource?

Could you explain the benefit in moving it if we plan to just move it back?

One observation here is that we've created two of these types that do the same thing with NoValueResponse<T> and ErrorResponse<T>. It would be good to consolidate and merge these as part of this effort. Until we do this, I feel strange about having our libraries take a dependency on one or the other.

ErrorResponse was created before NullableResponse and therefore doesn't inherit from it, so I don't see them as duplicate. That one seems fine to move, IMO.

[annelo-msft]

Could you explain the benefit in moving it if we plan to just move it back?

Sure. We have a lot of challenges with our shared source files today, many related to the issues listed in the PR description. I'd like to take an accounting of each of these files to understand what roles they play, what dependencies they actually have (some of them are referenced by libraries that don't actually use them), and how they're connected to each other. All of this is driving toward turning these into public types so they can be shared in the way .NET intends to share types. Moving them temporarily helps tease out the dependencies, and uses the CIs to validate hypotheses to ensure we don't make mistaken assumptions. It'll help us take out some of the snarly tangles in the circular dependency problem across the autorest.csharp repo so we can see that more clearly. Overall, the smaller the set of files in the src/Shared folder is, the easier it will be to see patterns to make it clear what the work remaining is here. I'm hoping to bring a list of projects to our team in a few weeks as options for people to consider driving during MQ.

So, if it's not a big problem to move it back in a bit, that would be really helpful for this effort. I'm happy to keep track of things we want to move back however works best for everyone.

ErrorResponse was created before NullableResponse and therefore doesn't inherit from it, so I don't see them as duplicate. That one seems fine to move, IMO.

I'm not sure I understand this - ErrorResponse<T> inherits from Response<T>, and so inherits from NullableResponse<T> transitively from that. If you understand this differently, could you help me understand your perspective?

Thanks!

[christothes]

Sure. We have a lot of challenges with our shared source files today, many related to the issues listed in the PR description. I'd like to take an accounting of each of these files to understand what roles they play, what dependencies they actually have (some of them are referenced by libraries that don't actually use them), and how they're connected to each other. All of this is driving toward turning these into public types so they can be shared in the way .NET intends to share types. Moving them temporarily helps tease out the dependencies, and uses the CIs to validate hypotheses to ensure we don't make mistaken assumptions. It'll help us take out some of the snarly tangles in the circular dependency problem across the autorest.csharp repo so we can see that more clearly. Overall, the smaller the set of files in the src/Shared folder is, the easier it will be to see patterns to make it clear what the work remaining is here. I'm hoping to bring a list of projects to our team in a few weeks as options for people to consider driving during MQ.

So, if it's not a big problem to move it back in a bit, that would be really helpful for this effort. I'm happy to keep track of things we want to move back however works best for everyone.

Do we have a timeline for when it would move back?

ErrorResponse was created before NullableResponse and therefore doesn't inherit from it, so I don't see them as duplicate. That one seems fine to move, IMO.

I'm not sure I understand this - ErrorResponse<T> inherits from Response<T>, and so inherits from NullableResponse<T> transitively from that. If you understand this differently, could you help me understand your perspective?

Thanks!

Sure - Response<T> overrides HasValue to be true and does not mark the Value property as nullable. Therefore, types inheriting directly from it should have a Value. Because it inherits from Response<T>, ErrorResponse<T> returns true for HasValue but will always throw when Value is accessed.

Response types that intend to have no value should inherit directly from NullableResponse<T> as the Value property is marked nullable and the derrived type can decide what the value of HasValue should be.

[annelo-msft]

Do we have a timeline for when it would move back?

Whenever we're ready to make it a public type? Or if there's a reason to share it across libraries before that, if we need to compromise on public/shared. Or, as soon as the accounting is done, which I'm hoping is only a few weeks, but should be before EOY, probably during MQ? What's best for your timeline?

Response types that intend to have no value should inherit directly from NullableResponse as the Value property is marked nullable and the derrived type can decide what the value of HasValue should be.

I see, cool, thanks for that. So then I was thinking about it incorrectly - it looks like ErrorResponse<T> is a duplicate of NoBodyResponse<T>, not NoValueResponse<T>. NoBodyResponse<T> is used in both Storage and AppConfig right now, so I can't move it out, but I think it would be worth merging it with ErrorResponse<T> and making one of them a public type if that makes sense.

[christothes]

Do we have a timeline for when it would move back?

Whenever we're ready to make it a public type? Or if there's a reason to share it across libraries before that, if we need to compromise on public/shared. Or, as soon as the accounting is done, which I'm hoping is only a few weeks, but should be before EOY, probably during MQ? What's best for your timeline?

MQ sounds reasonable - thanks for all the additional context.

[christothes]

tables LGTM