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

Fix graphql where clause when database schema is configured #16364

Merged
merged 8 commits into from
Jul 5, 2024

Conversation

mdameer
Copy link
Contributor

@mdameer mdameer commented Jun 23, 2024

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 use Dialect.QuoteForAliasName(tableAlias).

@hishamco
Copy link
Member

Can we have a unit test for that if it's possible

@mdameer
Copy link
Contributor Author

mdameer commented Jun 24, 2024

Can we have a unit test for that if it's possible

Sure I will do

@mdameer
Copy link
Contributor Author

mdameer commented Jun 24, 2024

@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.

@hishamco
Copy link
Member

What about a functional test?

@mdameer
Copy link
Contributor Author

mdameer commented Jun 27, 2024

What about a functional test?

@hishamco Done

@hishamco
Copy link
Member

I will have a look ASAP

@mdameer mdameer requested a review from hishamco June 29, 2024 21:23
@hishamco
Copy link
Member

@hyzx86 @gvkries do you have any feedback on this?

Co-authored-by: Tony Han <hyzx86@live.com>
Copy link
Contributor

@gvkries gvkries left a 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.

@hyzx86
Copy link
Contributor

hyzx86 commented Jul 1, 2024

Omitting the database schema looks suspicious to me. The schema is added before the table name correctly in YesSql, like $"[{schema}].[{tableName}]".

Good point!
@mdameer Can we make sure that the architecture was developed at the same time as the table alias was created?

@mdameer
Copy link
Contributor Author

mdameer commented Jul 1, 2024

@hyzx86 @gvkries hope I understood correctly. This area deals with table aliases, not table names. Adding the schema to a table alias is incorrect, so I switched the implementation to quote the table alias instead. If this doesn't address the issue, please provide further details.

@gvkries
Copy link
Contributor

gvkries commented Jul 2, 2024

@hyzx86 @gvkries hope I understood correctly. This area deals with table aliases, not table names. Adding the schema to a table alias is incorrect, so I switched the implementation to quote the table alias instead. If this doesn't address the issue, please provide further details.

I see, thanks for the explanation.

@hishamco
Copy link
Member

hishamco commented Jul 3, 2024

Anything to be added here @gvkries? What about your last suggestion #16364 (comment)

@gvkries
Copy link
Contributor

gvkries commented Jul 3, 2024

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 ;-)

@mdameer mdameer requested a review from gvkries July 4, 2024 19:57
@hishamco hishamco merged commit 52dca20 into OrchardCMS:main Jul 5, 2024
5 checks passed
@hishamco
Copy link
Member

hishamco commented Jul 5, 2024

Thanks for your contribution @mdameer

@mdameer mdameer deleted the 14627-graphql-schema branch July 5, 2024 16:29
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

Successfully merging this pull request may close these issues.

Can't use Where clause in GraphIQL
4 participants