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 type checking when importing types from recipes #15979 #15980

Merged
merged 24 commits into from
May 9, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
19889be
add typepart check #15979
hyzx86 May 5, 2024
34d55c0
Update ContentDefinitionService.cs
hyzx86 May 5, 2024
bd2ff8b
Merge branch 'main' into recipetypecheck
hyzx86 May 5, 2024
0f14c07
Update ContentDefinitionStep.cs
hyzx86 May 5, 2024
5018c0f
Update src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/C…
hyzx86 May 6, 2024
34063ce
Update test/OrchardCore.Tests/Recipes/RecipeFiles/recipe6.json
hyzx86 May 6, 2024
9f8809c
Cleaning up recipes and removing unwanted elements
May 6, 2024
a3b7a3c
use Assert.ThrowsAsync
May 6, 2024
0c2c3f3
Merge branch 'main' into recipetypecheck
hyzx86 May 6, 2024
3bc6cf2
Update src/OrchardCore.Modules/OrchardCore.ContentTypes/RecipeSteps/C…
hyzx86 May 6, 2024
d7d3851
Update test/OrchardCore.Tests/Recipes/RecipeExecutorTests.cs
hyzx86 May 6, 2024
fcf0ee7
Update test/OrchardCore.Tests/Recipes/RecipeFiles/recipe6.json
hyzx86 May 6, 2024
6483f83
Update test/OrchardCore.Tests/Recipes/RecipeFiles/recipe6.json
hyzx86 May 6, 2024
23aafad
Update test/OrchardCore.Tests/Recipes/RecipeExecutorTests.cs
hyzx86 May 6, 2024
f252446
Fix test
MikeAlhayek May 6, 2024
683b05a
Merge branch 'main' into recipetypecheck
hyzx86 May 6, 2024
f7cbf66
add notify
May 7, 2024
68d9b66
Merge branch 'main' into recipetypecheck
hyzx86 May 7, 2024
05387fd
update notify
hyzx86 May 7, 2024
e2e7a9e
ad log
hyzx86 May 7, 2024
174ec9c
Merge branch 'main' into recipetypecheck
hyzx86 May 8, 2024
bdd313d
restore Changes
hyzx86 May 8, 2024
75b42b6
Merge branch 'recipetypecheck' of https://github.com/hyzx86/OrchardCo…
hyzx86 May 8, 2024
13c32f1
remove localize
hyzx86 May 8, 2024
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
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using System.Text.Json.Nodes;
using System.Threading.Tasks;
using Microsoft.Extensions.Localization;
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved
using OrchardCore.ContentManagement.Metadata;
using OrchardCore.ContentManagement.Metadata.Models;
using OrchardCore.ContentManagement.Metadata.Records;
Expand All @@ -15,10 +16,11 @@ namespace OrchardCore.ContentTypes.RecipeSteps
public class ContentDefinitionStep : IRecipeStepHandler
{
private readonly IContentDefinitionManager _contentDefinitionManager;

public ContentDefinitionStep(IContentDefinitionManager contentDefinitionManager)
private readonly IStringLocalizer S;
public ContentDefinitionStep(IContentDefinitionManager contentDefinitionManager, IStringLocalizer<ContentDefinitionStep> s)
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed? If so, change s to stringLozalizer

{
_contentDefinitionManager = contentDefinitionManager;
S = s;
}

public async Task ExecuteAsync(RecipeExecutionContext context)
Expand Down Expand Up @@ -58,6 +60,11 @@ private Task UpdateContentTypeAsync(ContentTypeDefinition type, ContentTypeDefin

foreach (var part in record.ContentTypePartDefinitionRecords)
{
if (string.IsNullOrEmpty(part.PartName))
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved
{
throw new InvalidOperationException(S["Unable to add content-part to the '{0}' content-type. The part name cannot be null or empty.", type.Name]);
MikeAlhayek marked this conversation as resolved.
Show resolved Hide resolved
}

builder.WithPart(part.Name, part.PartName, partBuilder => partBuilder.MergeSettings(part.Settings));
}
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.IO;
using System.IO.Compression;
using System.Text.Json;
Expand Down Expand Up @@ -91,6 +92,10 @@ public async Task<IActionResult> Import(IFormFile importedPackage)

await _notifier.SuccessAsync(H["Deployment package imported."]);
}
catch (Exception ex)
{
await _notifier.ErrorAsync(H[ex.Message]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @MikeAlhayek ,Can you help me see what's wrong with my localization? Thank you!

Copy link
Member

Choose a reason for hiding this comment

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

you can't do that. the use of H and S must be a literal string since this has to be extracted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the notifier needs a HtmlLocalizedString ... What should I do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I can send the notify in Step logic?

This comment was marked as outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a layer of exception catching message alerts, as there is currently no exception handling when a recipe is imported.

you can't do that. the use of H and S must be a literal string since this has to be extracted.

Since the string is already processed internally, no additional processing should be required

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to write the exception to the user using notifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think you need to write the exception to the user using notifier.

I'm not sure I agree with this. Assuming that the person who has access to import recipes has full administrative access to the system, (I can't actually guess what other person roles have access to import recipes)
it makes sense to throw some underlying exceptions directly to the user, as most Windows applications do, so that the user can view the exceptions directly through the page rather than having to log in to the server to view the logs.
(I've developed a special feature to view log files online for this purpose~)

Currently our recipe import page does nothing, which would make the user feel that the system is very unreliable After all, we can't anticipate all exceptions

#15978

Copy link
Member

Choose a reason for hiding this comment

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

You can submit another PR to address another issue. I suggest keeping this PR simple and doing that type check without worrying about exception handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, got it. Thanks.

}
finally
{
if (System.IO.File.Exists(tempArchiveName))
Expand Down Expand Up @@ -148,6 +153,10 @@ public async Task<IActionResult> Json(ImportJsonViewModel model)

await _notifier.SuccessAsync(H["Recipe imported."]);
}
catch (Exception ex)
{
await _notifier.ErrorAsync(H[ex.Message]);
}
finally
{
if (Directory.Exists(tempArchiveFolder))
Expand Down
2 changes: 2 additions & 0 deletions test/OrchardCore.Tests/OrchardCore.Tests.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
<None Remove="Recipes\RecipeFiles\recipe3.json" />
<None Remove="Recipes\RecipeFiles\recipe4.json" />
<None Remove="Recipes\RecipeFiles\recipe5.json" />
<None Remove="Recipes\RecipeFiles\recipe6.json" />
</ItemGroup>

<ItemGroup>
Expand All @@ -44,6 +45,7 @@
<EmbeddedResource Include="Recipes\RecipeFiles\recipe3.json" />
<EmbeddedResource Include="Recipes\RecipeFiles\recipe4.json" />
<EmbeddedResource Include="Recipes\RecipeFiles\recipe5.json" />
<EmbeddedResource Include="Recipes\RecipeFiles\recipe6.json" />
</ItemGroup>

<ItemGroup>
Expand Down
22 changes: 22 additions & 0 deletions test/OrchardCore.Tests/Recipes/RecipeExecutorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using OrchardCore.Recipes.Models;
using OrchardCore.Recipes.Services;
using OrchardCore.Scripting;
using OrchardCore.Tests.Apis.Context;

namespace OrchardCore.Recipes
{
Expand Down Expand Up @@ -50,6 +51,27 @@ public async Task ShouldTrimValidScriptExpression(string recipeName, string expe
});
}

[Fact]
public async Task ContentDefinitionStep_WhenPartNameIsMissing_ThrowInvalidOperationException()
{
var context = new BlogContext();
await context.InitializeAsync();
await context.UsingTenantScopeAsync(async scope =>
{
var recipeExecutor = scope.ServiceProvider.GetRequiredService<IRecipeExecutor>();
// Act
var executionId = Guid.NewGuid().ToString("n");
var recipeDescriptor = new RecipeDescriptor { RecipeFileInfo = GetRecipeFileInfo("recipe6") };

var exception = await Assert.ThrowsAsync<InvalidOperationException>(async () =>
{
await recipeExecutor.ExecuteAsync(executionId, recipeDescriptor, new Dictionary<string, object>(), CancellationToken.None);
});

Assert.Equal("Unable to add content-part to the 'Message' content-type. The part name cannot be null or empty.", exception.Message);
});
}

private static Task<ShellScope> GetScopeAsync() => ShellScope.Context.CreateScopeAsync();

private static ShellContext CreateShellContext() => new()
Expand Down
27 changes: 27 additions & 0 deletions test/OrchardCore.Tests/Recipes/RecipeFiles/recipe6.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"name": "Recipe6",
"displayName": "Recipe 6",
"description": "This recipe is designed to uncover potential bugs during the content-definition import process. It serves as a validation for the import functionality.",
"author": "Tony Han",
"website": "",
"version": "1.0.0",
"issetuprecipe": false,
"categories": [ "test" ],
"steps": [
{
"name": "ContentDefinition",
"ContentTypes": [
{
"Name": "Message",
"DisplayName": "Message",
"ContentTypePartDefinitionRecords": [
{
// "PartName": "TitlePart", // Test PartName validation.
"Name": "TitlePart"
}
]
}
]
}
]
}