MissingMethodException ODataOptions.get_QuerySettings() in v8.1.0

[AndreaCuneo]

Assemblies affected
v8.1.0

Describe the bug
After bumping from v8.0.12 to v8.1.0, downstream libraries are failing at runtime due to public interface breaking change, which looks has been introduced in #835.

Reproduce steps
Using Asp.Versioning.OData.ApiExplorer library triggered as mentioned in dotnet/aspnet-api-versioning#980.

Possibily more libraries who are using this public interface are going to have the same issue (quick Github search yield ~100 public projects).

Expected behavior
Since this library follows SemVer, I'd not expect breaking changes in a Minor release. I'd have expected either a v9.0.0 or a retrocompatible change.

If possible, please release v8.1.1 with a fix or a v9.0.0 deprecating v8.1.0 on Nuget.

Additional context

System.MissingMethodException: Method not found: 'Microsoft.OData.ModelBuilder.Config.DefaultQuerySettings Microsoft.AspNetCore.OData.ODataOptions.get_QuerySettings()'.
at Asp.Versioning.ApiExplorer.ODataApiDescriptionProvider.ExploreQueryOptions(IEnumerable`1 apiDescriptions)
at Asp.Versioning.ApiExplorer.ODataApiDescriptionProvider.OnProvidersExecuted(ApiDescriptionProviderContext context)
at Microsoft.AspNetCore.Mvc.ApiExplorer.ApiDescriptionGroupCollectionProvider.GetCollection(ActionDescriptorCollection actionDescriptors)
at Microsoft.AspNetCore.Mvc.ApiExplorer.ApiDescriptionGroupCollectionProvider.get_ApiDescriptionGroups()
at Swashbuckle.AspNetCore.SwaggerGen.SwaggerGenerator.GetSwaggerDocumentWithoutFilters(String documentName, String host, String basePath)
at Swashbuckle.AspNetCore.SwaggerGen.SwaggerGenerator.GetSwaggerAsync(String documentName, String host, String basePath)

[xuzhg]

@AndreaCuneo Thanks for reporting this. OData's still finalizing the support policy, and it's not in my control.
8.1.0 is the version containing such changes, otherwise, it should be 8.0.13.

We should highlight those changes in the release note later.

[julealgon]

@xuzhg / @habbes is there a strong reason for not following semantic versioning on this library? Are you trying to match some .NET package versioning strategy or is there something else to it?

This is not the first time that we bump versions in a way that really doesn't respect semver at all.

As a consumer, this just makes it harder to know when there are new features vs breaking changes in any package update, and makes the whole update process riskier for everybody. Package updates that would otherwise be seamless suddenly become dangerous endeavors due to runtime compatibility problems like this very issue here.

Would it be possible to reconsider how OData is doing versioning moving forward?

[robertmclaws]

Yep. I've seen this happen entirely too many times to not be dealt with. If the library breaks something, it needs a major version update. Full stop. You CANNOT break things in minor version updates. Over the past year, I've seen too many things broken between releases, reliability has to be guaranteed, and SemVer is the first line of defense to do it.

[AndreaCuneo]

@AndreaCuneo Thanks for reporting this. OData's still finalizing the support policy, and it's not in my control.
8.1.0 is the version containing such changes, otherwise, it should be 8.0.13.

@xuzhg thanks for answer but it's not clear to me.

Are you telling me that this project's release policy doesn't follow SemVer and instead it accepts introducing Breacking Changes in Minor releases?
It's also not clear to me why the de facto versioning standard is not used.

If it is so, I suggest to describe this clearly in the Readme.md so that downstream library developers and ISV are aware of it and avoid updating Minors safely and constraint downstream packages with Patch-only matches ( Version=[8.0,8.1) instead of the usual Version=[8,9) ).

Currently many devs are supposing that SemVer was applied to this project and Minors are supposed to be compatible, which isn't the case. Since this requires correct handling in downstream packages, proper guidance would be appreciated.

[xuzhg]

I'd like to update the framework target dependency using #794, since .NET has the following support policy:

.NET Version	End of support
netcoreapp3.1	Support ended December 13 2022
Net 5.0	Support ended May 10, 2022
Net 6.0	Nov 12, 2024

I'd like to use 9.x for the framework target updating (remove 3.1 and 5.0).

But, it's not clear for my PR.

What do you think about it?

[AndreaCuneo]

I think this is some backward thinking and is not solving the issue. The fact that you're reserving v9 for dropping deprecated frameworks/runtime has nothing to do with the fact that a Minor version broken downstream Applications. Atm I cannot upgrade Application to v8.1, getting the fixes it includes, because that version broken the interface. I've first to wait for all libraries to update first, both Public (api-versioning mentioned) and Private ones. So this is delaying the process of getting the core fixes in Applications. As others mentioned, this Project has the habit to break downstream project/applications making an hurdle obtaining the fixes/improvements released as require extensive downstream work. In comparison the removal of runtimes removed upstream (Microsoft) is less impactful than the API change of this issue. I'd not complain if upstream deprecations are taken in a Minor honestly... Back to resolving this issue, I see these possible approaches: 1. Re-add the removed Property in ODataOptions, with ObsoleteAttribute and postpone its removal to v9. This means releasing v8.1.1 with that change and flag v8.1.0 as deprecated on Nuget as a bugged release. 2. Release as-is as v9.0.0, flagging v8.1.0 as deprecated on Nuget to avoid bumps. This means most will keep on v8.0 and not get the other fixes you added in v8.1. Dropping outdated runtimes is going to be v10 or v9.1. (9.1 is acceptable imho as only affects applications still running deprecated runtimes which should upgrade anyway. those who are on compatible runtimes has no issues) 3. revert the PR that requires API change, release as v8.2.0 and flag v8.1.0 as deprecated on Nuget. 4. Clarify in Readme.md that this Project doesn't follow SemVer standard warning downstream developers to use [X.Y, X.Y+1) constraint instead of the usual [X,X+1).

[habbes]

@xuzhg @julealgon @AndreaCuneo

I agree that making a breaking change in a minor release was not the right thing to do.

I agree with the proposal by @AndreaCuneo to deprecate v8.1.0 on NuGet and create a new release with the breaking changes undone (i.e. restoring the ODataOptions.QuerySettings property).

For the improvements that were introduce, I would propose either:

• having both QuerySettings and QueryConfigurations properties but marking QuerySettings as obsolete. Then internally you can make both compatibles, such that if the user updates the query settings, these values would be passed to the query configuration, and the query configuration would be used internally. Or some other pattern to retain backwards compatibility but, This is maintenance overhead, but it's the pattern that we have used in other scenarios where we wanted to introduce an improved API without breaking backwards compatibility (e.g.: https://github.com/OData/odata.net/pull/2431/files#diff-e8e6c154faa709040e266dad66878ebc7eccdc05a45fa2b801bb353edec83880R62)
• adding the new properties to the existing DefaultQuerySettings class in the model builder, although this seems less preferrable

I believe we should strive to adhere to semver. If there are exceptions, that should be communicated way in advance.

[julealgon]

That's some good stuff @habbes , thanks for sharing.

I'm honestly more worried about adoption of OData than individual concerns I might personally have with the versioning mishaps: when people start to get these types of unexpected breaks, trust in the library drops quite a lot as there is a perception of brittleness that is inherent to these problems, and this in turn reduces existing adoption and makes new folks stay away from it even more.

I think following SemVer properly would be a good start to improve on this external trust factor a bit more, but unfortunately OData hasn't been following SemVer for years now.

[corranrogue9]

@mikepizzo to follow up with @xuzhg

[commonsensesoftware]

Glad to see the versioning discussion get flushed out. I've been bit by such behavior at least a half dozen times since the 5.x timeframe. While all of the rules and policies about Semantic Versioning as well as binary versioning hold true, there are few additional things to consider that I'll chime in on.

First and foremost, OData (e.g this project) is built upon ASP.NET Core. Since 1.0, ASP.NET Core as a hard affinity to the .NET runtime version. If you want to support ASP.NET Core, then you must target the same TFM and major version. While it might work if you don't, there is no guarantee of that. It is rare, but I have already seen it happen in API Versioning when people have tried to use it with unofficially supported runtimes. As it stands, OData does not officially support .NET 7 as there is no corresponding TFM matching the version. The fact that it works is happenstance. It is a very bad idea to assume it will just work.

As far as I can tell, this project does not have a documented versioning strategy. If it followed the rest of the .NET platform, it would probably not be needed; however, since it does not, then it should be stated. It would appear that the goal is to align to the latest .NET LTS version. That is fine, but it should be clear that you are not officially supporting the latest .NET STS version. If you intend to support both, then you should be multi-targeting. This is low effort and guarantees things will work. A sound versioning policy should also state which versions you intend to support and for how long. The team can decide how flexible they want to be, but supporting just the latest .NET STS and LTS versions will align to most package consumers needs. These should each expect to align to a major .NET version. Aligning version numbers to ASP.NET Core and .NET is great, but not a requirement. Ensuring things don't break is far more important than making the numbers line up. Removing TFMs is a bit trickier. Ideally, that should correlate to a major version, but that doesn't always make sense. Clear communication as to when they will be removed goes a long way.

When you do create new major versions, it's a good idea create a permanent release branch that you can use to service that version. You need look no further than the .NET or ASP.NET Core branches. There are a number of tags for various releases, but that is not enough IMHO. A release branch will make it a lot easier to service patches. It's a lot harder to create arbitrary branches from tags (which is possible) and make it known they are the serviceable branches. All too often, a patch may have be serviced in one branch and cherry-picked into another. Ideally, it would be a RI or FI merge, but that isn't always the case. It's up to the team as to how many such branches exist, but I would expect to ~10 branches that cover the range of major versions and perhaps a few minor versions as well; something like release/<major>.<minor>. At some point, unserviced branches could be deleted. They can always be recreated from tags.

Finally, there is still a significant amount of manual configuration in the build process and hard dependencies on Visual Studio. These are absolutely unnecessary. The fact that the NuGet packing process is still using *.nuspec files is beyond belief and rift for mistakes (which I've been hit by in the past). There should be no part of the build that requires Visual Studio. Unless there is some very compelling reason, there should be no reason to have a hard dependency on Windows either. As an OSS project, there will still be many developers and contributors that do not use Visual Studio, are more comfortable with Visual Studio Code, and/or may not run on Windows. Avoiding these hard dependencies solves a lot of issues, even if you always run on them. I have offered before, and I will offer again, I'm willing to help the team remove this technical debt, but not without commitments from the stakeholders. @xuzhg has previously stated that this is an OSS project that anyone can contribute to. That is true, but there is no guarantee or commitment to accept the changes. The things that need to be fixed are not insignificant. Speaking for myself, I can say that my time is already impacted so much that there is no way I'll consider putting forth the effort to make things better if there is not a commitment to resolve these issues. As much I was want to help the team, and the community more so, I'm not going to put spend days worth of work so it can sit in PR Limbo and hope it gets approved sometime in the next 2 years. There are still open PRs that have been sitting there for more than a year. If they are abandoned or will never be accepted, then they should be closed. I'm not asking for pre-approval, lack of discussion, or lack of review. All I ask is commitment to drive it to the end state.

[commonsensesoftware]

To zero in on what led us to this place, @xuzhg, and perhaps @mikepizzo, can you explain the rationale behind #834? That seems to be what resulted in this breaking change. From what I can tell, it appears to be purely philosophical for the purposes of a minor or patch version, but with public, breaking changes. The new DefaultQueryConfigurations is exactly the same as DefaultQuerySettings.

DefaultQuerySettings comes from the Model Builder package so it seems that the intent was to introduce DefaultQueryConfigurations so that it can evolve independently; for example, new members can be added in the future. That is all fine and well, but why did it have to happen now? There is currently no advantage whatsoever.

The first option would have been to simply defer this change until it was absolutely necessary. At that point, you could decide how you would handle the fork in the road.

The second option would have been to make this completely backward compatible with a warning about obsolescence. For example:

public class DefaultQueryConfigurations : DefaultQuerySettings
{
}

Extend the base class so it's backward compatible. This inheritance chain can be broken in the next major version. Then you would update the options to be:

public class ODataOptions
{
    [Obsolete("QuerySettings will be removed in the next major version. Use QueryConfigurations instead.")]
    public DefaultQuerySettings QuerySettings => QueryConfigurations;

    public DefaultQueryConfigurations QueryConfigurations { get; } = new();
}

This would enable full backward compatibility, while still introducing your new change. The true breaking change can occur in 9.0+ when the QuerySettings property is removed.

The topic of #785 is a bit irrelevant to the problem at hand, but it should be addressed. What should have happened IMHO is making sure you have a release that aligns with the .NET release cadence, even if there is no implementation changes. That would solve how and when to drop old TFMs. You're in a bit of a conundrum now so you either need to follow through with what you have until .NET 8 in Nov '23 or make a very clear announcement as to when and how you will drop it. It's not ideal, but it isn't completely unreasonable to drop a TFM in a minor version. You're not breaking anything as much as you are dropping a particular implementation. I'm sure some people will disagree and you will have to weigh the cost. If you had clear release branches as mentioned above, there would be an escape hatch for people that might need to fork the code/branch because they are stuck on an unsupported runtime.

[AndreaCuneo]

@corranrogue9 @xuzhg any update/plan on this?

[xuzhg]

@commonsensesoftware I'd like to take your suggestion in the PR #891.

Meanwhile, after releasing 8.1.2, i will deprecate the 8.1.0 and 8.1.1 using the following message:

@AndreaCuneo let us know if that's what you are looking for. Thanks.

[commonsensesoftware]

@xuzhg The changes look good. Thanks for the quick response. Given the scenario, your package version deprecation plan seems reasonable to me.

[AndreaCuneo]

@xuzhg thanks for the news.

Code-side I trust @commonsensesoftware.
I suggest to include a reference to this issue in the deprecation message.

I'm available to test pre-release packages.

[xuzhg]

@commonsensesoftware Would you please help verify this version working fine?

[AndreaCuneo]

@xuzhg I tested it and confirm I don't hit the error anymore.