U4-10409 - UDI Parsing should not require scanning assemblies

Created by Shannon Deminick 13 Sep 2017, 04:09:26 Updated by Sebastiaan Janssen 13 Sep 2017, 14:18:50

Tags: Unscheduled

Subtask of: U4-9609

This is critical code and it should perform the least amount of friction possible. In almost all cases there is no reason to perform assembly scanning and we really don't need to be so strict on what types are available.

This also leads to problems when trying to parse a UDI in non-normal circumstances like running code in pre-startup stages.

Comments

Shannon Deminick 13 Sep 2017, 04:20:19

PR: https://github.com/umbraco/Umbraco-CMS/pull/2191

  • Removes scanning assemblies whenever ParseInternal is called
  • We have a list of known UDI types in the core so those are still loaded in
  • When we encounter a UDI in ParseInternal that we are not aware of, then we will determine that based on parsing a GUID and if that doesn't work we'll deem it to be a string (there doesn't seem to be a reason to have 'unknown')
  • Removes the very strict restrictions on creating UDIs and determining valid UDI types - why do we care?
  • Only scan assemblies for known types based on IServiceConnectors if we need to return a root UDI - this seems to be the only relevant place where this might be required


Stephan 13 Sep 2017, 06:05:52

see also: https://stackoverflow.com/questions/4626647/asp-net-this-method-cannot-be-called-during-the-applications-pre-start-initial/38126685#38126685


Stephan 13 Sep 2017, 06:21:22

Reason to have UdiType.Unknown is that there is no "default" UdiType - common practice with enums: set the first value to be Unknown - which will be integer value zero, so anything that's not initialized will be "Unknown".


Stephan 13 Sep 2017, 07:35:24

Fixed:

  • removed reflection when getting known types
  • fixed parsing a range which would still fail

Caveats:

  • anything that involves parsing root udis still breaks
  • parsing an invalid udi (eg "umb://should-be-guid/") corrupts KnownUdiTypes
  • parsing an ambiguous udi (eg "umb://should-be-string/") fails and corrupts KnowUdiTypes


Stephan 13 Sep 2017, 07:42:15

The real issue is probably that we are doing way too many things in pre-start but we're not going to fix it now - so... I'm ok living with the first two caveats - but the third one annoys me as it can lead to some very weird issue where a string udi with an ambiguous path would break stuff?


Stephan 13 Sep 2017, 10:08:30

Sooo... have kinda refactored...

  • extend Core Udi to support parsing without scanning (parsing for "known types"). by default we still scan and fail on unknown types, but we now can skip scanning and return a special Udi for unkown types. we don't "discover" types and there's no guessing based on path. unless you explicitely want to parse for known types only, we're 100% unchanged.

  • extend Deploy with DiskService.ReadKnownTypeArtifacts() that is used in DatabaseHelper to read artifacts of type that are known at that time, and ignore anything else - because anyways, those that are not know at that time can be entirely ignored. this is using the new parsing.

This should fix the original issue without introducing the risks I mentioned above?

There's a PR for Deploy too: https://github.com/umbraco/Umbraco-Deploy/pull/175

So both new Core and Deploy are required for it to work. It's done via Cms compatibility, so the new Deploy still supports the old Cores (but of course the issue is not fixed).


Sebastiaan Janssen 13 Sep 2017, 14:18:50

FYI made KnownTypeUdiJsonConverter intenral as it's a class that needs to be removed.


Priority: Major

Type: Backlog Item

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 7.6.7

Sprint: Sprint 67

Story Points: 0.5

Cycle: