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

SQL queries in GraphQL do not return results anymore #16201

Closed
gvkries opened this issue May 31, 2024 · 16 comments · Fixed by #16234
Closed

SQL queries in GraphQL do not return results anymore #16201

gvkries opened this issue May 31, 2024 · 16 comments · Fixed by #16234
Labels
Milestone

Comments

@gvkries
Copy link
Contributor

gvkries commented May 31, 2024

Describe the bug

There is a regression in the latest version (main branch). A SQL query does not return any results when used in a GraphQL query.

To Reproduce

  1. Create a new site with the blog recipe.
  2. Create a second blog post and edit both post a few times by saving drafts, publishing etc.
  3. Change the RecentBlogPosts query to return documents.
  4. Use the query in GraphiQL. It does return an empty array.

Screenshots

image

It looks like _session.GetAsync<ContentItem>() does not return anything.

@gvkries
Copy link
Contributor Author

gvkries commented May 31, 2024

This must be something else, not sure what the error is. It's not showing up with a new site.

@Piedone
Copy link
Member

Piedone commented May 31, 2024

In the repro you said to try with a new site, but then you commented it's not applicable to a new site. Which one is the repro?

@gvkries
Copy link
Contributor Author

gvkries commented May 31, 2024

Yeah, I'm super confused ATM 🙈

@gvkries
Copy link
Contributor Author

gvkries commented May 31, 2024

To Reproduce

  1. Create a new site with the blog recipe.
  2. Create a second blog post and edit both post a few time by saving drafts, publishing etc.
  3. Change the RecentBlogPosts query to return documents.
  4. Use the query in GraphiQL. It does return an empty array.

So, this error occurs after some editing has been done. Seems to be related to versioning the items or something like that.

(PS.: I updated the original repro steps as well.)

@Piedone
Copy link
Member

Piedone commented May 31, 2024

There's this simpler repro:

  1. Set up with Blog.
  2. Enable GraphQL.
  3. Change the RecentBlogPosts query to return documents.
  4. Add a new Blog Post (just fill the title, publish).
  5. Observe in GraphiQL that the below query only returns the first Post, coming from the recipe.
query MyQuery {
  recentBlogPosts {
    displayText
  }
}
{
  "data": {
    "recentBlogPosts": [
      {
        "displayText": "Man must explore, and this is exploration at its greatest"
      }
    ]
  }
}

Running the SQL Query directly (i.e. go to Queries, Run) correctly returns both Posts. Nothing seems suspicious:

[
    {
        "Id": 15,
        "DocumentId": 18,
        "ContentItemId": "4f78wgf3hdfgvsh78vha32c1wy",
        "ContentItemVersionId": "40157dyvtp1tp2mgb77t2jzrw2",
        "Latest": 1,
        "Published": 1,
        "ContentType": "BlogPost",
        "ModifiedUtc": "2024-05-31 12:45:45.2139249",
        "PublishedUtc": "2024-05-31 12:45:45.2267931",
        "CreatedUtc": "2024-05-31 12:45:45.2139249",
        "Owner": "464qygsbznkmrswe1jsdqe3kxs",
        "Author": "admin",
        "DisplayText": "Demo"
    },
    {
        "Id": 10,
        "DocumentId": 10,
        "ContentItemId": "4c5nda7x5rgq1v8mtxv654rgmz",
        "ContentItemVersionId": "42pm5ntv0zxjswvm0161x8p4m8",
        "Latest": 1,
        "Published": 1,
        "ContentType": "BlogPost",
        "ModifiedUtc": "2024-05-31 12:45:00.5585245",
        "PublishedUtc": "2024-05-31 12:45:00.5664133",
        "CreatedUtc": "2024-05-31 12:45:00.5568733",
        "Owner": "464qygsbznkmrswe1jsdqe3kxs",
        "Author": "admin",
        "DisplayText": "Man must explore, and this is exploration at its greatest"
    }
]

@Piedone Piedone added this to the 2.0 milestone May 31, 2024
Copy link
Contributor

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

@gvkries
Copy link
Contributor Author

gvkries commented May 31, 2024

And I did not find when this broke 🤷‍♂️

@Piedone Piedone assigned Piedone and unassigned Piedone May 31, 2024
@Piedone
Copy link
Member

Piedone commented May 31, 2024

The issue is that (in my above example) the documentIds contains [15, 10]. However, those are not the Documents' IDs, but the Id values of the ContentItemIndex rows. We should rather take the DocumentId column from there (which are 10 and 18, so one of the posts only works by coincidence).

The issue is that connection.QueryAsync<long>(rawQuery, parameters, transaction) will take the first column that matches long, which for new installations will be Id.

Changing the query to this fixes the issue:

SELECT DocumentId FROM ContentItemIndex WHERE ContentType='BlogPost' ORDER BY CreatedUtc DESC LIMIT 3

However, I'm unsure about how to tackle this:

  • We can change the query in the recipe, and instruct people to make their queries always return only a Document ID, which should've always been the case. I'd do this, since with the long cast matching the first column, a * query (which is inefficient anyway) is uncertain anyway.
  • Try to fish out the DocumentId from the query results with some heuristics. However, we need to keep in mind that the query can be anything, for any table (including custom ones), and the only thing we can reasonably assume is that there will be a column that contains Document IDs. This might have any random name, and be long or int. There can be any number of columns with these types that aren't the Document IDs.

What do you think?

@gvkries
Copy link
Contributor Author

gvkries commented May 31, 2024

Oh, I should have seen that. I only checked the "return documents" field and it started to work. So I never actually checked the query itself 🙈 I think this is not a bug then...

Thanks a lot for your help!

@gvkries
Copy link
Contributor Author

gvkries commented May 31, 2024

I don't think it's worth changing anything here. Note to myself: one must create a correct query :)

@gvkries gvkries closed this as completed May 31, 2024
@Piedone
Copy link
Member

Piedone commented May 31, 2024

I think this is indeed a bug, just not in SQL Query, but perhaps the recipe (though it only really surfaces an issue if you try to use it with GraphQL) and and an omission in the documentation.

@Piedone
Copy link
Member

Piedone commented May 31, 2024

So, I think this docs section should include that the query should select a column containing a document ID. And the Query in the recipe should select DocumentId, since that's needed in any case, regardless if you use it in GraphQL or not.

@gvkries
Copy link
Contributor Author

gvkries commented Jun 1, 2024

The query in the recipe does not return documents, so it's correct on its own IMHO. But maybe we can detect those errors and make it harder for people to fall into the same trap as I did.

@Piedone
Copy link
Member

Piedone commented Jun 1, 2024

It's not incorrect but it's unnecessary, since for its default usage in a template you also only need the document ID.

We could also add a hint to the checkbox BTW.

Not sure but maybe we need something similar for Lucene and Elasticsearch Queries too.

@Piedone Piedone modified the milestones: 2.0, 2.x Jun 1, 2024
Copy link
Contributor

github-actions bot commented Jun 1, 2024

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

@gvkries
Copy link
Contributor Author

gvkries commented Jun 4, 2024

@Piedone The blog recipe actually used a wrong property name in the SQL query step. I also made the Sql query source a little bit more forgiving by trying to get the correct column. Only if no DocumentId column is in the query, it falls back to the first long column it finds.

@MikeAlhayek MikeAlhayek modified the milestones: 2.x, 2.0 Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants