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

RegexAdjustRelativePaths does not take account of cache file being stored on different url (i.e. blob storage) #10

Open
I3undy opened this issue Dec 1, 2015 · 11 comments

Comments

@I3undy
Copy link
Member

I3undy commented Dec 1, 2015

rdobson created on Sep 3, 2014:
https://combinator.codeplex.com/workitem/66

I have changed to the following:

private static void RegexAdjustRelativePaths(CombinatorResource resource, Uri cacheFileUri)
{
    RegexProcessUrls(resource,
        (match) =>
        {
            var url = match.Groups[1].ToString();

            var uri = RegexMakeInlineUri(resource, url);

            // Remote paths are preserved as full urls, local paths become uniformed relative ones.
            var uriString = "";
            if (resource.IsCdnResource || resource.AbsoluteUrl.Host != uri.Host || resource.AbsoluteUrl.Port != uri.Port)
                uriString = uri.ToProtocolRelative();
            else if (cacheFileUri != null && (cacheFileUri.Host != uri.Host || cacheFileUri.Port != uri.Port))
                uriString = uri.ToProtocolRelative();
            else
                uriString = uri.PathAndQuery;

            return "url(\"" + uriString + "\")";
        });
}

I am passing in a uri of a fake file in storage to check against using the following method in CacheFileService:

public Uri GetUri() {
        try {
            var publicUrl = _storageProvider.GetPublicUrl("test.txt");
            return new Uri(publicUrl);
        }
// ReSharper disable once EmptyGeneralCatchClause
        catch {
        }
        return null;
    }
@I3undy
Copy link
Member Author

I3undy commented Dec 1, 2015

@Piedone commented on Sep 3, 2014:

Hmm, it should. It works also on DotNest and all of the Lombiq sites (these use Azure Blob storage).

Do I understand correctly that the main change is that this now also checks for the port (as it only checks for the host now) to determine whether the URL of the stylesheets and the resources linked from inside it are under the same domain? Because I don't think it would be good practice to put css files under a different port then e.g. images linked from it if the host is the same.

@I3undy
Copy link
Member Author

I3undy commented Dec 1, 2015

rdobson commented on Sep 3, 2014:

Without this check comparing inline resources to the cache storage location references to resources under the website itself become broken as they do not exist in the relative blob storage location but actually exist as part of the orchard site itself.

@I3undy
Copy link
Member Author

I3undy commented Dec 1, 2015

rdobson commented on Sep 3, 2014:

The port number check is there for the azure emulator which is on the same host but the blob storage is on port 10000, without this it is impossible to test and debug a site in the emulator with combinator enabled.

@I3undy
Copy link
Member Author

I3undy commented Dec 1, 2015

@Piedone commented on Sep 5, 2014:

So you experience this when running the Azure Orchard solution, right?

@I3undy
Copy link
Member Author

I3undy commented Dec 1, 2015

rdobson commented on Sep 8, 2014:

Yes that's correct

@I3undy
Copy link
Member Author

I3undy commented Dec 1, 2015

@Piedone commented on Sep 8, 2014:

I'll need to think about this, I don't particularly like the usage of a dummy storage file. Wouldn't GetUri() be unneeded with the modification in #6 since then resource.AbsoluteUrl is already the correct URL of the resource?

@I3undy
Copy link
Member Author

I3undy commented Dec 1, 2015

rdobson commented on Sep 8, 2014:

I don't like the dummy storage file either (although it never actually ever exists), its the only way I could see of getting the base public uri for where the storage is (and thus where the combined content is served from), it would be cleaner is IStorageProvider exposed an extra method or maybe would work by passing null to GetPublicUrl.

In the azure solution everything stored using IStorageProvider is served directly from blob storage rather than the webrole itself which is were most of the problem occurs, the combinator does not seem to expect this to be the case, hence the additional check needed to check if it is, otherwise it wrongly maps resource urls breaking them.

@I3undy
Copy link
Member Author

I3undy commented Dec 1, 2015

@Piedone commented on Sep 8, 2014:

I see, but wouldn't resource.AbsoluteUrl contain the correct URL with the blob storage port already?

@I3undy
Copy link
Member Author

I3undy commented Dec 1, 2015

@Piedone commented on Sep 8, 2014:

With your modification from the other issue I mean?

@I3undy
Copy link
Member Author

I3undy commented Dec 1, 2015

rdobson commented on Sep 8, 2014:

Not for resources that are hosted in the webrole, without seeing where the cache files are hosted it tries to use uri.PathAndQuery for those which is not correct if the cache file is hosted outside of the webrole the urls become broken, its got nothing to do with ports at that point, the resources hosted on the webrole need to have absolute url's if the cache file is outside of the webrole, not relative ones.

@I3undy
Copy link
Member Author

I3undy commented Dec 1, 2015

rdobson commented on Sep 8, 2014:

To put it another way if there is a url quite legitimately of /Module/Styles/image.png it will stay as that in the blob cache file which would then be trying to reference http://storageaccount.blob.core.windows.net/Module/Styles/image.png rather than the correct url of http://www.website.com/Module/Styles/image.png.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant