Asset URL Replacement Enhancement

[xantari]

Motivation

Which issue does this fix? Fixes #311, #193, and #397

If no issue exists, what is the fix or new feature? Were there any reasons to fix/implement things that are not obvious?

Checklist

• Code follows coding conventions held in this repo
• Automated tests have been added
• Tests are passing
• Docs have been updated (if applicable)
• Temporary settings (e.g. variables used during development and testing) have been reverted to defaults

How to test

If manual testing is required, what are the steps?

N/A

[anthony-sams]

Looks good! Url replacement strategy is future base url change resistant as well

[xantari]

@pokornyd What is status on this?

[pokornyd]

hello and thank you for the PR submission. while this implementation is valid, it only covers standalone asset elements. for assets used in rich text, you'd need to implement additional handling at RichTextContentConverter level

I would also refactor the replacement function to parse the original asset url as URI and append URI.PathAndQuery to the user provided custom domain, instead of counting the slashes.

feel free to extend the PR to cover rich text elements as well, otherwise, I'll look into the matter myself, though I can't promise a release until new year

[xantari]

I'll see if I can figure out the RichTextResolver piece you mentioned.

In regards to the URI route, I did a quick performance test and it appears that the string version I proposed is about 2.74x faster then the URI approach. So not sure if you still want to do it that way?

Here is the benchmark program: PerfTestUrlReplacement.zip

BenchmarkDotNet v0.14.0, Windows 10 (10.0.19045.5131/22H2/2022Update)
Intel Core i9-10885H CPU
2.40GHz, 1 CPU, 16 logical and 8 physical cores
.NET SDK 9.0.100
[Host] : .NET 8.0.11 (8.0.1124.51707), X64 RyuJIT AVX2 [AttachedDebugger]
DefaultJob : .NET 8.0.11 (8.0.1124.51707), X64 RyuJIT AVX2

Method	Mean	Error	StdDev
TestWithString	85.11 ns	1.577 ns	1.475 ns
TestWithUri	233.90 ns	2.325 ns	2.175 ns

[xantari]

@pokornyd I updated the rich text content converter as well. It appears that it only is used when you try to embed images into rich text blocks. Let me know if I missed anything. I left the string replacement version in place for now until you can review the performance question above.

[xantari]

@pokornyd Any update?

[pokornyd]

hello Xantari, thanks for the additional input. please accept my apologies regarding the delayed replies, I had to prioritize work on other repositories and didn't get a chance to review your proposed changes yet. I'll put this on top of my to-do list and get back to you asap, thanks for your patience.

[pokornyd]

@pokornyd I updated the rich text content converter as well. It appears that it only is used when you try to embed images into rich text blocks. Let me know if I missed anything. I left the string replacement version in place for now until you can review the performance question above.

The performance gain is still in the realm of nanoseconds, so I would say that it's negligible in this scenario and prefer readability over minimal performance gain. Feel free to commit the URI version and I'll review the remaining code in the meantime.

As for the rich text image support, I think it's a valid scenario, as images in rich text element are commonplace and would benefit from the custom asset domain support as well.