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

Add IContextCache to deploy connectors #13287

Merged
merged 7 commits into from Oct 25, 2022

Conversation

ronaldbarendse
Copy link
Contributor

This adds the IContextCache that was added to Deploy 4.7.0, 9.5.0 and 10.1.0 (see PR https://github.com/umbraco/Umbraco-Deploy/pull/673) and updates the connector interfaces that are part of the CMS to use this.

These changes are backwards compatible for callers, but breaking for the implementations to ensure the IContextCache overload is always used. Updating implementations to be compatible again is as simple as adding the new IContextCache contextCache parameter, as the previous (now obsolete) method will then fallback to the interface default implementation using PassThroughCache.Instance.

@ronaldbarendse ronaldbarendse added category/breaking status/dependency-update This change requires a dependency to be updated (Deploy, Forms, Courier) labels Oct 24, 2022
Copy link
Contributor

@AndyButland AndyButland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Approving, but just one thing to consider... do we we want the implementations for IContextCache in core? I can see you need to have PassThroughCache, but the other two might be better in Deploy, so we can more easily make changes to them should we ever need to.

You'll need to liaise too please with @bergmania about getting these changes into CMS, as we'll need to put out an RC2 in order to be able to take a dependency on them.

@bergmania
Copy link
Member

I agree with Andy, about moving the deploy specific implementations to deploy.

@AndyButland AndyButland merged commit 1560e84 into v11/dev Oct 25, 2022
@AndyButland AndyButland deleted the v11/feature/deploy-icontextcache branch October 25, 2022 16:40
nikolajlauridsen pushed a commit that referenced this pull request Oct 26, 2022
* Add IContextCache and implementations

* Update connector interfaces to use IContextCache

* Minor cleanup

* Move DeployContextCache prefix to constant

* Move default implementations to obsolete methods

* Remove DeployContextCache and DictionaryCache

Co-authored-by: Andy Butland <abutland73@gmail.com>
Zeegaan added a commit that referenced this pull request Nov 8, 2022
* Bump version

* Add IContextCache to deploy connectors (#13287)

* Add IContextCache and implementations

* Update connector interfaces to use IContextCache

* Minor cleanup

* Move DeployContextCache prefix to constant

* Move default implementations to obsolete methods

* Remove DeployContextCache and DictionaryCache

Co-authored-by: Andy Butland <abutland73@gmail.com>

* Add IContextCache to deploy connectors (#13287)

* Add IContextCache and implementations

* Update connector interfaces to use IContextCache

* Minor cleanup

* Move DeployContextCache prefix to constant

* Move default implementations to obsolete methods

* Remove DeployContextCache and DictionaryCache

Co-authored-by: Andy Butland <abutland73@gmail.com>

* Parse lockId as invariant (#13284)

Co-authored-by: Zeegaan <nge@umbraco.dk>

* Fix Sqlite database locking issue (#13246)

* Add locking for creating scope

* Lock the repository instead

* Add scope in action instead of locking in service

* Fix up post-merge

Co-authored-by: Zeegaan <nge@umbraco.dk>

* Bump version to next minor

* Fix for UseExceptionHandler no longer working since v10.3 RC (#13218)

* Fix for UseExceptionHandler no longer working since v10.3 RC

* Update the management api path to match the new one

Co-authored-by: Nikolaj <nikolajlauridsen@protonmail.ch>

* New backoffice: Cleanup management API routes (#13296)

* Rename ModelsBuilderDashboard folder to ModelsBuilder

* Fix modelsbuilder paths and related naming

* Rename analytics route to telemetry

* Fix controller bases - routes and tags

* Fix items route

* Fix more controllerbase routes

* Fix route

* Fix OpenApi file

* Merging DictionaryItem and Dictionary

* Fix TrackedReferences naming

* Update OpenApi file

* Rename Analytics* related types to Telemetry*

* New Backoffice: Return AnalyticsLevelViewModel from Telemetry/ (#13298)

* Return TelemetryLevelViewModel instead of TelemetryLevel

* Fix schema

* Change telemetry/current to telemetry/level

(cherry picked from commit f2b8494)

* Add contants for tree and recycle-bin subpaths

(cherry picked from commit 4449f56)

Co-authored-by: Mole <nikolajlauridsen@protonmail.ch>

* Updated Smidge, Npoco and MailKit (#13310)

* Updated Smidge, Npoco and MailKit

* Added missing command (after breaking interface in npoco)

* OpenId Connect authentication for new management API (#13318)

* First attempt at OpenIddict

* Making headway and more TODOs

* Redo current policies for multiple schemas + clean up auth controller

* Fix bad merge

* Clean up some more test code

* Fix spacing

* Include AddAuthentication() in OpenIddict addition

* A little more clean-up

* Move application creation to its own implementation + prepare for middleware to handle valid callback URL

* Enable refresh token flow

* Fix bad merge from v11/dev

* Support auth for Swagger and Postman in non-production environments + use default login screen for back-office logins

* Add workaround to client side login handling so the OAuth return URL is not corrupted before redirection

* Add temporary configuration handling for new backoffice

* Restructure the code somewhat, move singular responsibility from management API project

* Add recurring task for cleaning up old tokens in the DB

* Fix bad merge + make auth controller align with the new management API structure

* Explicitly handle the new management API path as a backoffice path (NOTE: this is potentially behaviorally breaking!)

* Redo handle the new management API requests as backoffice requests, this time in a non-breaking way

* Add/update TODOs

* Revert duplication of current auth policies for OpenIddict (as it breaks everything for V11 without the new management APIs) and introduce a dedicated PoC policy setup for OpenIddict.

* Fix failing unit tests

* Update src/Umbraco.Cms.ManagementApi/Security/BackOfficeApplicationManager.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* Update src/Umbraco.Cms.ManagementApi/Security/BackOfficeApplicationManager.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* Update src/Umbraco.Cms.ManagementApi/Security/BackOfficeApplicationManager.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* Update src/Umbraco.Core/Routing/UmbracoRequestPaths.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* V11: Using IFileProvider to access assets added from packages (#13141)

* Creating a FileProviderFactory for getting the package.manifest and grid.editors.config.js files through a file provider

* Collecting the package.manifest-s from different sources

* Searching different sources for grid.editors.config.js

* Using an IFileProvider to collect all tours

* Refactoring IconService.cs

* Typo

* Optimizations when looping through the file system

* Moving WebRootFileProviderFactory to Umbraco.Web.Common proj

* Removes double registering

* pluginLangFileSources includes the localPluginFileSources

* Comments

* Remove linq from foreach

* Change workflow for grid.editors.config.js so we check first physical file, then RCL, then Embedded

* Clean up

* Check if config dir exists

* Discover nested package.manifest files

* Fix IFileInfo.PhysicalPath check

* Revert 712810e as that way files in content root are preferred over those in web root

* Adding comments

* Refactoring

* Remove PhysicalPath check

* Fix registration of WebRootFileProviderFactory

* Use Swashbuckle instead of NSwag (#13350)

* First attempt at OpenIddict

* Making headway and more TODOs

* Redo current policies for multiple schemas + clean up auth controller

* Fix bad merge

* Clean up some more test code

* Fix spacing

* Include AddAuthentication() in OpenIddict addition

* A little more clean-up

* Move application creation to its own implementation + prepare for middleware to handle valid callback URL

* Enable refresh token flow

* Fix bad merge from v11/dev

* Support auth for Swagger and Postman in non-production environments + use default login screen for back-office logins

* Add workaround to client side login handling so the OAuth return URL is not corrupted before redirection

* Add temporary configuration handling for new backoffice

* Restructure the code somewhat, move singular responsibility from management API project

* Add recurring task for cleaning up old tokens in the DB

* Fix bad merge + make auth controller align with the new management API structure

* Explicitly handle the new management API path as a backoffice path (NOTE: this is potentially behaviorally breaking!)

* Redo handle the new management API requests as backoffice requests, this time in a non-breaking way

* Add/update TODOs

* Replace NSwag with Swashbuckle and clean up unnecessary client secret workaround

* Revert duplication of current auth policies for OpenIddict (as it breaks everything for V11 without the new management APIs) and introduce a dedicated PoC policy setup for OpenIddict.

* Fix failing unit tests

* A little niceness + export new OpenApi.json and fix path in contract unit test

* Redo after merge with v11/dev + filter out unwanted mime types

* Remove CreatedResult and NotFoundObjectResult where possible

* Custom schema IDs - no more "ViewModel" postfix and make generic lists look less clunky too

* A little more explanation for generic schema ID generation

* Force Swashbuckle to use enum string names

* Update OpenApi.json to match new enum string values

* Add clarifying comment about weird looking construct

* add workflow to schema (#13349)

* add workflow to schema

* add licenses to CMSDefinition - intentionally only adding to schema, not registered as options

Co-authored-by: Nikolaj <nikolajlauridsen@protonmail.ch>
Co-authored-by: Ronald Barendse <ronald@barend.se>
Co-authored-by: Andy Butland <abutland73@gmail.com>
Co-authored-by: Zeegaan <nge@umbraco.dk>
Co-authored-by: Justin Neville <67802060+justin-nevitech@users.noreply.github.com>
Co-authored-by: Elitsa Marinovska <21998037+elit0451@users.noreply.github.com>
Co-authored-by: Bjarke Berg <mail@bergmania.dk>
Co-authored-by: Kenn Jacobsen <kja@umbraco.dk>
Co-authored-by: Nathan Woulfe <nathan@nathanw.com.au>
@leekelleher
Copy link
Member

Hiya. I've finally got around to testing my Contentment package with Umbraco 11.0.0-rc5, and hit a breaking-change with the .ToArtifact method.

To reproduce, from the CLI, I installed a fresh Umbraco 11.0.0-rc5 site, ran the installer, backoffice loaded up, all good, then stopped the website and added the Contentment package...

  • dotnet add package Our.Umbraco.Community.Contentment
  • dotnet run

Then I got the following error...

Stack trace from dotnet run after adding Contentment to an Umbraco 11.0.0-rc5 instance
Unhandled exception. System.Reflection.ReflectionTypeLoadException: Could not load all types from "Umbraco.Community.Contentment, Version=4.2.0.0, Culture=neutral, PublicKeyToken=null" due to LoaderExceptions, skipping:
. System.TypeLoadException on Umbraco.Community.Contentment.DataEditors.EditorNotesConfigurationConnector: Method 'ToArtifact' in type 'Umbraco.Community.Contentment.DataEditors.EditorNotesConfigurationConnector' from assembly 'Umbraco.Community.Contentment, Version=4.2.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.
. System.TypeLoadException on Umbraco.Community.Contentment.DataEditors.NotesConfigurationConnector: Method 'ToArtifact' in type 'Umbraco.Community.Contentment.DataEditors.NotesConfigurationConnector' from assembly 'Umbraco.Community.Contentment, Version=4.2.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.
. System.TypeLoadException on Umbraco.Community.Contentment.DataEditors.RenderMacroConfigurationConnector: Method 'ToArtifact' in type 'Umbraco.Community.Contentment.DataEditors.RenderMacroConfigurationConnector' from assembly 'Umbraco.Community.Contentment, Version=4.2.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.

Method 'ToArtifact' in type 'Umbraco.Community.Contentment.DataEditors.EditorNotesConfigurationConnector' from assembly 'Umbraco.Community.Contentment, Version=4.2.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.
Method 'ToArtifact' in type 'Umbraco.Community.Contentment.DataEditors.NotesConfigurationConnector' from assembly 'Umbraco.Community.Contentment, Version=4.2.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.
Method 'ToArtifact' in type 'Umbraco.Community.Contentment.DataEditors.RenderMacroConfigurationConnector' from assembly 'Umbraco.Community.Contentment, Version=4.2.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.
   at Umbraco.Cms.Core.Composing.TypeFinder.GetTypesWithFormattedException(Assembly a)
   at Umbraco.Cms.Core.Composing.TypeFinder.GetClassesWithBaseType(Type baseType, IEnumerable`1 assemblies, Boolean onlyConcreteClasses, Func`2 additionalFilter)
   at Umbraco.Cms.Core.Composing.TypeFinder.FindClassesOfType(Type assignTypeFrom, IEnumerable`1 assemblies, Boolean onlyConcreteClasses)
   at Umbraco.Extensions.TypeFinderExtensions.FindClassesOfType[T](ITypeFinder typeFinder, IEnumerable`1 assemblies, Boolean onlyConcreteClasses)
   at Umbraco.Cms.Core.Composing.TypeLoader.<>c__DisplayClass21_0`1.<GetTypes>b__0()
   at Umbraco.Cms.Core.Composing.TypeLoader.GetTypesInternalLocked(Type baseType, Type attributeType, Func`1 finder, String action, Boolean cache)
   at Umbraco.Cms.Core.Composing.TypeLoader.GetTypesInternal(Type baseType, Type attributeType, Func`1 finder, String action, Boolean cache)
   at Umbraco.Cms.Core.Composing.TypeLoader.GetTypes[T](Boolean cache, IEnumerable`1 specificAssemblies)
   at Umbraco.Cms.Core.DependencyInjection.UmbracoBuilderExtensions.AddAllCoreCollectionBuilders(IUmbracoBuilder builder)
   at Umbraco.Cms.Core.DependencyInjection.UmbracoBuilder.AddCoreServices()
   at Umbraco.Cms.Core.DependencyInjection.UmbracoBuilder..ctor(IServiceCollection services, IConfiguration config, TypeLoader typeLoader, ILoggerFactory loggerFactory, IProfiler profiler, AppCaches appCaches, IHostingEnvironment hostingEnvironment)
   at Umbraco.Extensions.UmbracoBuilderExtensions.AddUmbraco(IServiceCollection services, IWebHostEnvironment webHostEnvironment, IConfiguration config)
   at Umbraco11rc5.Startup.ConfigureServices(IServiceCollection services) in C:\VCS\Umbraco\Development\Umbraco11rc5\Startup.cs:line 32
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at Microsoft.AspNetCore.Hosting.ConfigureServicesBuilder.InvokeCore(Object instance, IServiceCollection services)
   at Microsoft.AspNetCore.Hosting.GenericWebHostBuilder.UseStartup(Type startupType, HostBuilderContext context, IServiceCollection services, Object instance)
   at Microsoft.AspNetCore.Hosting.GenericWebHostBuilder.<>c__DisplayClass14_0.<UseStartup>b__0(HostBuilderContext context, IServiceCollection services)
   at Microsoft.Extensions.Hosting.HostBuilder.InitializeServiceProvider()
   at Microsoft.Extensions.Hosting.HostBuilder.Build()
   at Umbraco.Cms.Web.Common.Hosting.UmbracoHostBuilderDecorator.Build()
   at Umbraco11rc5.Program.Main(String[] args) in C:\VCS\Umbraco\Development\Umbraco11rc5\Program.cs:line 6
System.TypeLoadException: Method 'ToArtifact' in type 'Umbraco.Community.Contentment.DataEditors.EditorNotesConfigurationConnector' from assembly 'Umbraco.Community.Contentment, Version=4.2.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.
System.TypeLoadException: Method 'ToArtifact' in type 'Umbraco.Community.Contentment.DataEditors.NotesConfigurationConnector' from assembly 'Umbraco.Community.Contentment, Version=4.2.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.
System.TypeLoadException: Method 'ToArtifact' in type 'Umbraco.Community.Contentment.DataEditors.RenderMacroConfigurationConnector' from assembly 'Umbraco.Community.Contentment, Version=4.2.0.0, Culture=neutral, PublicKeyToken=null' does not have an implementation.

From the changes on this PR, I can see that the Obsolete attribute has been added...

https://github.com/umbraco/Umbraco-CMS/pull/13287/files#diff-27975a4ec9d6d75aaaffce8ed8b4e4259989327ecce6df8cb8a032064400c9f1R30

...so I'm unsure why I'd get an unhandled exception on building against the Contentment package.

For reference, here's a link to the source of one of the ToArtifact calls: https://github.com/leekelleher/umbraco-contentment/blob/4.2.0/src/Umbraco.Community.Contentment/DataEditors/EditorNotes/EditorNotesConfigurationConnector.cs#L69

@bergmania
Copy link
Member

Hi @leekelleher.. We'll take a look.. Thanks for reporting

@leekelleher
Copy link
Member

Thanks @bergmania. I realized later that the IDataTypeConfigurationConnector interface has the new method points, thus breaking the implementation, (sounds obvious now 🤦). 😅

@ronaldbarendse
Copy link
Contributor Author

@leekelleher This PR indeed contains breaking changes for the implementations listed below, so you'll need to update these to be compatible for Umbraco 11.

Since Contentment is multi-targeting Umbraco based on .NET versions, this would mean you can't target .NET 7 against anything lower than Umbraco 11. This in turn means Umbraco 10 projects with Contentment installed can't upgrade their target framework to .NET 7 (similar to how Umbraco 9 projects currently can't upgrade to .NET 6 because of the minimum Umbraco 10 dependency that's on the Contentment .NET 6 build). Since Umbraco aligns with the .NET LTS versions, it doesn't make much sense to upgrade .NET versions without upgrading Umbraco, so this should be fine (for now, as Umbraco 12 will still target .NET 7, because .NET 8 has a projected release date of 2023-11, aligning with Umbraco 13 LTS).

Breaking changes

New method overloads were added to these interfaces:

  • IDataTypeConfigurationConnector
    • string? ToArtifact(IDataType dataType, ICollection<ArtifactDependency> dependencies, IContextCache contextCache)
    • object? FromArtifact(IDataType dataType, string? configuration, IContextCache contextCache)
  • IServiceConnector
    • IArtifact? GetArtifact(Udi udi, IContextCache contextCache)
    • IArtifact GetArtifact(object entity, IContextCache contextCache)
  • IValueConnector
    • string? ToArtifact(object? value, IPropertyType propertyType, ICollection<ArtifactDependency> dependencies, IContextCache contextCache)
    • object? FromArtifact(string? value, IPropertyType propertyType, object? currentValue, IContextCache contextCache)
  • IGridCellValueConnector
    • string? GetValue(GridValue.GridControl gridControl, ICollection<ArtifactDependency> dependencies, IContextCache contextCache)
    • void SetValue(GridValue.GridControl gridControl, IContextCache contextCache)

The corresponding overloads without the IContextCache parameter are obsoleted and have a default implementation that forwards calls using PassThroughCache.Instance.

@leekelleher
Copy link
Member

Thanks @ronaldbarendse - all understood. 👍

I must admit that I'm confused about the breaking-change policy for major versions (in terms of package support). I was lead to believe that if there was a proposed removal, that would be deprecated in v9, remain for v10 and removed in v11. Giving package developers time to align their codebases.

Would the same logic not apply to additions, (especially with interfaces)?

Seeing a similar discussion (from April) about new methods on interfaces, #12218 (comment), could we take a similar approach? e.g. keeping IDataTypeConfigurationConnector as is for v11, introducing IDataTypeConfigurationConnector2, remain for v12 and merge them in v13? (or maybe there's a way to do this with default interfaces, but I may be lacking C# knowledge there).

My main point is... that when someone is trying out the v11 RC, would we expect them to install existing packages (that supported v10), or expect them to be broken?

I can happily compile Contentment against v11/.NET7, (I've already done this), but it does feel like added pressure to actively support Umbraco's major version release cadence, which may become a big ask for free/open-source solo developers over time.

@bergmania
Copy link
Member

bergmania commented Nov 28, 2022

IMO we should do whatever is possible to avoid these breaking changes..

It seems like we can just add default implementations that all just throw NotImplementedException, then we are home safe on this.

I tested out Contentment when doing this for IDataTypeConfigurationConnector, and that seems to work. Not sure if there are any big disadvantages to that.

@bergmania
Copy link
Member

@ronaldbarendse, Another approach is to call the old signature from the new. That will lead to stackoverflowexceptions if non of the overloads is actually implemented, but that is not the case.
This will continue to work for existing packages?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/breaking release/11.0.0 status/dependency-update This change requires a dependency to be updated (Deploy, Forms, Courier)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants