-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix graphql where clause when database schema is configured #16364
Conversation
Can we have a unit test for that if it's possible |
Sure I will do |
@hishamco I forgot to mention that this issue affects the tenant when specifying a table schema during setup. As far as I know, SQLite doesn't support schemas. If you have any ideas on how we can implement unit tests for this service, please let me know. |
What about a functional test? |
@hishamco Done |
I will have a look ASAP |
test/OrchardCore.Tests/Apis/GraphQL/Queries/PredicateQueryTests.cs
Outdated
Show resolved
Hide resolved
test/OrchardCore.Tests/Apis/GraphQL/Queries/PredicateQueryTests.cs
Outdated
Show resolved
Hide resolved
test/OrchardCore.Tests/Apis/GraphQL/Queries/PredicateQueryTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Tony Han <hyzx86@live.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omitting the database schema looks suspicious to me. The schema is added before the table name correctly in YesSql, like $"[{schema}].[{tableName}]"
.
The error described in the bug report must be caused by something else.
Good point! |
src/OrchardCore/OrchardCore.ContentManagement.GraphQL/Queries/Predicates/PredicateQuery.cs
Outdated
Show resolved
Hide resolved
I see, thanks for the explanation. |
src/OrchardCore/OrchardCore.ContentManagement.GraphQL/Queries/Predicates/PredicateQuery.cs
Outdated
Show resolved
Hide resolved
Anything to be added here @gvkries? What about your last suggestion #16364 (comment) |
I think that is another bug and should be fixed, but someone with more insights to this code should have a look. @mdameer seems to have grokked this part ;-) |
Thanks for your contribution @mdameer |
Fix #14627
The old code
Dialect.QuoteForTableName(tableAlias, _configuration.Schema)
was adding the schema to a table alias (e.g., ContentItemIndex_a1), which caused the issue. I updated it to useDialect.QuoteForAliasName(tableAlias)
.