Improve GraphQL Query Schema validation and logging #6536

hyzx86 · 2020-06-29T18:37:27Z

No description provided.

src/OrchardCore.Modules/OrchardCore.OpenId/Configuration/OpenIdClientConfiguration.cs

deanmarcussen

Logging uses a different string interpolation method to prevent allocations, so you should never use $ to format log messages.

I think I'm ok with this staying as LogWarning

src/OrchardCore.Modules/OrchardCore.OpenId/Configuration/OpenIdClientConfiguration.cs

…dClientConfiguration.cs Co-authored-by: Dean Marcussen <[email protected]>

…patch-3

hyzx86 · 2020-07-01T15:34:03Z

src/OrchardCore.Modules/OrchardCore.Lucene/GraphQL/LuceneQueryFieldTypeProvider.cs

-                    var queryField = BuildContentTypeFieldType(schema, contentType, query);
-                    if (queryField != null)
+                    var querySchema = JObject.Parse(query.Schema);
+                    if (!querySchema.ContainsKey("type"))


We have to add a sample schema on the page ,but I don't know the right schema for here.

Please leave that for a seperate pr

deanmarcussen

@hyzx86 I have updated the logging statements with correct usage.

Please can you look at my comment on the controller error handling and see what the result is when adding a ModelState error. It should definitely use ModelState errors, it's possible you may have to have a notifier as well, but I would prefer it is all handled by ModelState errors.

Probably sorting that out is the last thing to do for this pr :)

deanmarcussen · 2020-07-03T07:10:51Z

src/OrchardCore.Modules/OrchardCore.Queries/Controllers/AdminController.cs

+            if (!string.IsNullOrWhiteSpace(query.Schema) && !CheckSchema(query.Schema))
+            {
+                _notifier.Error(H["Invalid schema JSON supplied."]);
+                model.Editor = await _displayManager.BuildEditorAsync(query, updater: _updateModelAccessor.ModelUpdater, isNew: true);


This should add just add a ModelState error, and see what the result is when it is handled later in the code

// If we got this far, something failed, redisplay form --> All errors should be handled by this section model.Editor = editor;

Hi @deanmarcussen , thanks for your help , ModelState added

deanmarcussen · 2020-07-03T07:11:15Z

src/OrchardCore.Modules/OrchardCore.Lucene/GraphQL/LuceneQueryFieldTypeProvider.cs

-                    var queryField = BuildContentTypeFieldType(schema, contentType, query);
-                    if (queryField != null)
+                    var querySchema = JObject.Parse(query.Schema);
+                    if (!querySchema.ContainsKey("type"))


Please leave that for a seperate pr

deanmarcussen · 2020-07-06T12:58:38Z

src/OrchardCore.Modules/OrchardCore.Queries/Controllers/AdminController.cs

-
+            if (!string.IsNullOrWhiteSpace(query.Schema) && !CheckSchema(query.Schema))
+            {
+                ModelState.AddModelError("Schema", S["Invalid schema JSON supplied."]);


Ok great, this works better.

Sorry, what I should have said though, is to implement IValidatableObject on the QueriesCreateViewModel.

Apply the test for valid JSON on that View Model by implementing IValidatableObject.

Here is an example from where it is used elsewhere in the code https://github.com/OrchardCMS/OrchardCore/blob/dev/src/OrchardCore.Modules/OrchardCore.Users/ViewModels/LinkExternalLoginViewModel.cs

I think you will need to apply the same to the edit controller QueriesEditViewModel View Model

Then in the Queries.Fields.Edit.cshtml View you should be able to apply
<span asp-validation-for="Schema"></span> (like is done for Name in the same file) and you will get a validation error on the schema field itself

But the value of Schema comes from query variable, not model, model.Schema is always empty

And the query object cannot be filled if it is verified during creation

Ok, I just added a string extension for testing json here #6614

But I have had a look at the Queries AdminController and understand what is driving that viewmodel now.

I was wrong to suggest IValidateableObject.

The query Schema, is actually managed and updated by the QueryDisplayDriver.cs

It already performs some checks for validity so the test you are doing for valid Json should move into the QueryDisplayDriver.cs UpdateAsync() method, along with the other checks for valid Names etc.

deanmarcussen

LGTM.

While this looks like a lot of formatting changes, it as actually because things are being wrapped in a try catch block to prevent errors being thrown when parsing json.

Tony Han and others added 6 commits June 22, 2020 18:44

add query schema validate

b3d3a7a

Change method name

38e54fc

merge from github

9ea0d09

Add record ValidateSettings failure reason

72f2dc8

Merge remote-tracking branch 'upstream/dev' into hyz-dev

6b92d4a

Added ValidateSettings failure reason log

1918d76

hishamco requested changes Jul 1, 2020

View reviewed changes

src/OrchardCore.Modules/OrchardCore.OpenId/Configuration/OpenIdClientConfiguration.cs Outdated Show resolved Hide resolved

Removes namespaces that are never used

9fb235d

hyzx86 requested a review from hishamco July 1, 2020 04:10

hishamco reviewed Jul 1, 2020

View reviewed changes

src/OrchardCore.Modules/OrchardCore.OpenId/Configuration/OpenIdClientConfiguration.cs Outdated Show resolved Hide resolved

deanmarcussen reviewed Jul 1, 2020

View reviewed changes

src/OrchardCore.Modules/OrchardCore.OpenId/Configuration/OpenIdClientConfiguration.cs Show resolved Hide resolved

hyzx86 and others added 2 commits July 1, 2020 16:59

Update OpenIdClientConfiguration.cs

e25d638

Log the reason for the validation failure

7aea2b9

deanmarcussen reviewed Jul 1, 2020

View reviewed changes

Tony Han added 2 commits July 1, 2020 22:24

Merge from prev commit

c3b2c09

Validate query schema , and record schema check log.

609f2fd

hyzx86 mentioned this pull request Jul 1, 2020

add query schema validate ，and skip error query schema #6477

Closed

Tony Han and others added 6 commits July 1, 2020 22:35

log openId config errors

53615ca

Fix ErrorLog

4a34084

Update src/OrchardCore.Modules/OrchardCore.OpenId/Configuration/OpenI…

b17dcb5

…dClientConfiguration.cs Co-authored-by: Dean Marcussen <[email protected]>

fix the log format

e8b610e

Merge branch 'patch-3' of https://github.com/hyzx86/OrchardCore into …

b161fe9

…patch-3

Update notify Invalid schema JSON supplied

4b7f3dd

hyzx86 changed the title ~~Added ValidateSettings failure reason log~~ Added ValidateSettings failure reason log , add query schema validate ，and skip error query schema Jul 1, 2020

Tony Han added 2 commits July 1, 2020 23:03

remove unused namespace

fcc13a8

Fix Logs error

446087c

hyzx86 commented Jul 1, 2020

View reviewed changes

hyzx86 requested review from deanmarcussen and hishamco July 2, 2020 13:05

Update LuceneQueryFieldTypeProvider.cs

b3d1072

deanmarcussen added 3 commits July 3, 2020 07:57

Update LuceneQueryFieldTypeProvider.cs

c78bbc4

Update OpenIdClientConfiguration.cs

b2ac68e

Update SqlQueryFieldTypeProvider.cs

f0027a6

deanmarcussen requested changes Jul 3, 2020

View reviewed changes

Tony Han added 4 commits July 4, 2020 18:22

Add Model Error

cb65a51

Fix Model state error

57cc321

add AddModelError with localstring

be1fe6b

Add Line

5af6fc5

hyzx86 requested a review from deanmarcussen July 4, 2020 11:06

remove duplicate notice

d4ea34f

deanmarcussen reviewed Jul 6, 2020

View reviewed changes

deanmarcussen mentioned this pull request Jul 9, 2020

Add and implement IsJson() string extension method #6614

Merged

Tony Han added 2 commits July 14, 2020 00:59

Merge remote-tracking branch 'upstream/dev' into patch-3

7b38c6c

move check schema to QueryDisplayDriver

9e91764

hyzx86 requested a review from deanmarcussen July 13, 2020 17:46

Update QueryDisplayDriver.cs

8f83997

deanmarcussen approved these changes Jul 16, 2020

View reviewed changes

deanmarcussen changed the title ~~Added ValidateSettings failure reason log , add query schema validate ，and skip error query schema~~ Improve GraphQL Query Schema validation and logging Jul 16, 2020

agriffard merged commit 920d002 into OrchardCMS:dev Jul 17, 2020

agriffard mentioned this pull request Aug 13, 2020

Edit Content Part, Add New Field, Kills GraphQL #5536

Closed

hyzx86 deleted the patch-3 branch June 13, 2024 06:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve GraphQL Query Schema validation and logging #6536

Improve GraphQL Query Schema validation and logging #6536

hyzx86 commented Jun 29, 2020

deanmarcussen left a comment

hyzx86 Jul 1, 2020 •

edited

Loading

hyzx86 Jul 2, 2020

deanmarcussen Jul 3, 2020

deanmarcussen left a comment

deanmarcussen Jul 3, 2020

hyzx86 Jul 4, 2020

This comment was marked as off-topic.

deanmarcussen Jul 3, 2020

deanmarcussen Jul 6, 2020

hyzx86 Jul 6, 2020

hyzx86 Jul 6, 2020

deanmarcussen Jul 9, 2020

deanmarcussen left a comment

Improve GraphQL Query Schema validation and logging #6536

Improve GraphQL Query Schema validation and logging #6536

Conversation

hyzx86 commented Jun 29, 2020

deanmarcussen left a comment

Choose a reason for hiding this comment

hyzx86 Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deanmarcussen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deanmarcussen left a comment

Choose a reason for hiding this comment

hyzx86 Jul 1, 2020 •

edited

Loading