Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

* Redesign CompilationResult so that it does not throw when CompiledType is accessed #2045

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/Microsoft.AspNet.Mvc.Core/Properties/Resources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@

using System;
using System.Collections.Generic;
using Microsoft.AspNet.Diagnostics;
using System.Linq;
using Microsoft.Framework.Runtime;
using Microsoft.Framework.Internal;

namespace Microsoft.AspNet.Mvc.Razor
{
/// <summary>
/// An exception thrown when accessing the result of a failed compilation.
/// An <see cref="Exception"/> thrown when accessing the result of a failed compilation.
/// </summary>
public class CompilationFailedException : Exception, ICompilationException
{
Expand All @@ -20,12 +21,19 @@ public class CompilationFailedException : Exception, ICompilationException
/// details of the compilation failure.</param>
public CompilationFailedException(
[NotNull] ICompilationFailure compilationFailure)
: base(Resources.FormatCompilationFailed(compilationFailure.SourceFilePath))
: base(FormatMessage(compilationFailure))
{
CompilationFailures = new[] { compilationFailure };
}

/// <inheritdoc />
public IEnumerable<ICompilationFailure> CompilationFailures { get; }

private static string FormatMessage(ICompilationFailure compilationFailure)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We changed Diagnostics \ Helios to not print compilation messages. So we can do better with showing exception messages in the debugger now.

{
return Resources.CompilationFailed +
Environment.NewLine +
string.Join(Environment.NewLine, compilationFailure.Messages.Select(message => message.FormattedMessage));
}
}
}
53 changes: 0 additions & 53 deletions src/Microsoft.AspNet.Mvc.Razor/Compilation/CompilationFailure.cs

This file was deleted.

57 changes: 0 additions & 57 deletions src/Microsoft.AspNet.Mvc.Razor/Compilation/CompilationMessage.cs

This file was deleted.

101 changes: 25 additions & 76 deletions src/Microsoft.AspNet.Mvc.Razor/Compilation/CompilationResult.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.IO;
using Microsoft.AspNet.FileProviders;
using Microsoft.Framework.Runtime;
using Microsoft.Framework.Internal;

namespace Microsoft.AspNet.Mvc.Razor
Expand All @@ -14,8 +12,6 @@ namespace Microsoft.AspNet.Mvc.Razor
/// </summary>
public class CompilationResult
{
private Type _type;

/// <summary>
/// Creates a new instance of <see cref="CompilationResult"/>.
/// </summary>
Expand All @@ -24,109 +20,62 @@ protected CompilationResult()
}

/// <summary>
/// Gets the path of the Razor file that was compiled.
/// Gets (or sets in derived types) the type produced as a result of compilation.
/// </summary>
public string FilePath
{
get
{
if (File != null)
{
return File.PhysicalPath;
}
return null;
}
}
/// <remarks>This property is <c>null</c> when compilation failed.</remarks>
public Type CompiledType { get; protected set; }

/// <summary>
/// Gets a sequence of <see cref="CompilationMessage"/> instances encountered during compilation.
/// Gets (or sets in derived types) the generated C# content that was compiled.
/// </summary>
public IEnumerable<CompilationMessage> Messages { get; private set; }
public string CompiledContent { get; protected set; }

/// <summary>
/// Gets (or sets in derived types) the generated C# content that was compiled.
/// Gets the <see cref="ICompilationFailure"/> produced from parsing or compiling the Razor file.
/// </summary>
public string CompiledContent { get; protected set; }
/// <remarks>This property is <c>null</c> when compilation succeeded.</remarks>
public ICompilationFailure CompilationFailure { get; private set; }

/// <summary>
/// Gets (or sets in derived types) the type produced as a result of compilation.
/// Gets the <see cref="CompilationResult"/>.
/// </summary>
/// <exception cref="CompilationFailedException">An error occured during compilation.</exception>
public Type CompiledType
/// <returns>The current <see cref="CompilationResult"/> instance.</returns>
/// <exception cref="CompilationFailedException">Thrown if compilation failed.</exception>
public CompilationResult EnsureSuccessful()
{
get
{
if (_type == null)
{
throw CreateCompilationFailedException();
}

return _type;
}
protected set
if (CompilationFailure != null)
{
_type = value;
throw new CompilationFailedException(CompilationFailure);
}
}

private IFileInfo File { get; set; }
return this;
}

/// <summary>
/// Creates a <see cref="CompilationResult"/> that represents a failure in compilation.
/// Creates a <see cref="CompilationResult"/> for a failed compilation.
/// </summary>
/// <param name="fileInfo">The <see cref="IFileInfo"/> for the Razor file that was compiled.</param>
/// <param name="compilationContent">The generated C# content to be compiled.</param>
/// <param name="messages">The sequence of failure messages encountered during compilation.</param>
/// <returns>A CompilationResult instance representing a failure.</returns>
public static CompilationResult Failed([NotNull] IFileInfo file,
[NotNull] string compilationContent,
[NotNull] IEnumerable<CompilationMessage> messages)
/// <param name="compilationFailure">The <see cref="ICompilationFailure"/> produced from parsing or
/// compiling the Razor file.</param>
/// <returns>A <see cref="CompilationResult"/> instance for a failed compilation.</returns>
public static CompilationResult Failed([NotNull] ICompilationFailure compilationFailure)
{
return new CompilationResult
{
File = file,
CompiledContent = compilationContent,
Messages = messages,
CompilationFailure = compilationFailure
};
}

/// <summary>
/// Creates a <see cref="CompilationResult"/> that represents a success in compilation.
/// Creates a <see cref="CompilationResult"/> for a successful compilation.
/// </summary>
/// <param name="type">The compiled type.</param>
/// <returns>A CompilationResult instance representing a success.</returns>
/// <returns>A <see cref="CompilationResult"/> instance for a successful compilation.</returns>
public static CompilationResult Successful([NotNull] Type type)
{
return new CompilationResult
{
CompiledType = type
};
}

private CompilationFailedException CreateCompilationFailedException()
{
var fileContent = ReadContent(File);
var compilationFailure = new CompilationFailure(FilePath, fileContent, CompiledContent, Messages);
return new CompilationFailedException(compilationFailure);
}

private static string ReadContent(IFileInfo file)
{
try
{
using (var stream = file.CreateReadStream())
{
using (var reader = new StreamReader(stream))
{
return reader.ReadToEnd();
}
}
}
catch (Exception)
{
// Don't throw if reading the file fails.
return string.Empty;
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ private GetOrAddResult OnCacheMiss(RelativeFileInfo file,
string normalizedPath,
Func<RelativeFileInfo, CompilationResult> compile)
{
var compilationResult = compile(file);
var compilationResult = compile(file).EnsureSuccessful();

// Concurrent addition to MemoryCache with the same key result in safe race.
var cacheEntry = _cache.Set(normalizedPath,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using Microsoft.AspNet.FileProviders;

namespace Microsoft.AspNet.Mvc.Razor
{
/// <summary>
Expand All @@ -13,11 +11,11 @@ public interface ICompilationService
/// <summary>
/// Compiles content and returns the result of compilation.
/// </summary>
/// <param name="fileInfo">The <see cref="IFileInfo"/> for the Razor file that was compiled.</param>
/// <param name="fileInfo">The <see cref="RelativeFileInfo"/> for the Razor file that was compiled.</param>
/// <param name="compilationContent">The generated C# content to be compiled.</param>
/// <returns>
/// A <see cref="CompilationResult"/> representing the result of compilation.
Copy link
Member

Choose a reason for hiding this comment

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

nit: "... for the compilation."

/// </returns>
CompilationResult Compile(IFileInfo fileInfo, string compilationContent);
CompilationResult Compile(RelativeFileInfo fileInfo, string compilationContent);
Copy link
Member

Choose a reason for hiding this comment

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

why is this cleanup part of this PR?

Copy link
Member

Choose a reason for hiding this comment

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

put better, how is move from IFileInfo to RelativeFileInfo related to avoiding exceptions in CompilationResult?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's part of the work tiem - #955 (comment). We were relying of PhysicalPath to create diagnositcs which isn't guaranteed to be available.

Copy link
Member

Choose a reason for hiding this comment

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

👌

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System.Collections.Generic;
using Microsoft.Framework.Runtime;
using Microsoft.Framework.Internal;

namespace Microsoft.AspNet.Mvc.Razor
{
/// <summary>
/// <see cref="ICompilationFailure"/> for Razor parse failures.
/// </summary>
public class RazorCompilationFailure : ICompilationFailure
Copy link
Member

Choose a reason for hiding this comment

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

can't see how this class and RazorCompilationMessage changed after you renamed them. could you please temporarily revert the rename to make other changes visible? fine to leave the class names as-is during this interim review phase.

{
/// <summary>Initializes a new instance of <see cref="RazorCompilationFailure"/>.</summary>
/// <param name="sourceFilePath">The path of the Razor source file that was compiled.</param>
/// <param name="sourceFileContent">The contents of the Razor source file.</param>
/// <param name="messages">A sequence of <see cref="ICompilationMessage"/> encountered
/// during compilation.</param>
public RazorCompilationFailure(
[NotNull] string sourceFilePath,
[NotNull] string sourceFileContent,
[NotNull] IEnumerable<RazorCompilationMessage> messages)
{
SourceFilePath = sourceFilePath;
SourceFileContent = sourceFileContent;
Messages = messages;
}

/// <inheritdoc />
public string SourceFilePath { get; }

/// <inheritdoc />
public string SourceFileContent { get; }

/// <inheritdoc />
public string CompiledContent { get; } = null;

/// <inheritdoc />
public IEnumerable<ICompilationMessage> Messages { get; }
}
}
Loading