Skip to content

QueryServer: Add support for new query-server #1508

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

Merged
merged 8 commits into from
Oct 12, 2022
Merged

Conversation

aeisenberg
Copy link
Contributor

This is a new variant of #1499 that is based on my most recent changes to #1498.

@aeisenberg aeisenberg requested a review from a team as a code owner September 13, 2022 21:50
@aeisenberg aeisenberg force-pushed the alexet/prepare-new-qs branch from ab25ea1 to e5fe214 Compare September 13, 2022 21:55
@aeisenberg aeisenberg marked this pull request as draft September 13, 2022 21:56
Ensures better backwards compatibility with legacy query objects.
@aeisenberg aeisenberg marked this pull request as ready for review September 23, 2022 15:44
@aeisenberg aeisenberg requested a review from a team as a code owner September 23, 2022 18:14
@aeisenberg aeisenberg changed the base branch from alexet/prepare-new-qs to main September 26, 2022 23:24
@aeisenberg aeisenberg requested a review from alexet October 3, 2022 14:39
@aeisenberg
Copy link
Contributor Author

@alexet will you be able to take a look at this? It's largely based on your original PR with some added things on top.

@alexet
Copy link
Contributor

alexet commented Oct 4, 2022

I am not fully sure on the evaluation result changes. The problem is that the two messages look similar but are not compatible in meaning so the union is actually useless without some key saying which case we are in. In fact originally I didn't have evaluation result at all on the results but to handle serialisation I needed to handle the old one. Is there a reason why it is needed?

@aeisenberg
Copy link
Contributor Author

The reason why I added that was for backwards compatibility. If you upgrade to the new version of the extension and then downgrade again and you have a saved query from the new version, then your query history will fail to load and your extension will fail to load as well. This is a bit of an edge case since we don't expect most users to ever downgrade.

The reason for the error is that the new history does not serialize the evaluation result and the old extension will fail on a type error.

Alternatively, I can just fill it with blank data so that the old extension can handle new history without crashing.

I also need to think better about migrating query history. I will create a new issue for this.

@aeisenberg aeisenberg force-pushed the aeisenberg/new-qs branch 2 times, most recently from 3046e90 to d461372 Compare October 4, 2022 21:10
@alexet
Copy link
Contributor

alexet commented Oct 6, 2022

I didn't think about downgrading. We should fill it in with something. I think we want to just convert everything to success/other-error with the old error codes.

We probably should be more lenient about query history loading. At worst a malformed entry should just lose a single history item. However that doesn't help us in this case.

I personally also disliked the method of loading json and setting a prototype as it felt very hacky.

@aeisenberg
Copy link
Contributor Author

I didn't think about downgrading. We should fill it in with something. I think we want to just convert everything to success/other-error with the old error codes.

That's mostly what the latest commit does. There are two new error codes that are not in legacy-messages. The older extension handles this gracefully. Would you be fine if I converted DBSCHEME_MISMATCH_NAME and DBSCHEME_NO_UPGRADE to OTHER_ERROR? (Note that this is essentially how it is being handled in the currently released extension).

We probably should be more lenient about query history loading. At worst a malformed entry should just lose a single history item. However that doesn't help us in this case.

The error doesn't happen during loading. That part of the code just discards any obviously malformed history entries. It doesn't do a complete consistency check, though. And later on when the item is about to be displayed, there is an instruction that fails if there is no result. I should probably also add some more complete checks to the deserialization.

I personally also disliked the method of loading json and setting a prototype as it felt very hacky.

I agree, but I didn't see any way around this without a massive rewrite. Query history items and some of their properties always had a custom prototype. I thought it was safer to edit the prototype after creation instead of rewriting everything to use standard objects.

@aeisenberg aeisenberg removed the request for review from a team October 6, 2022 14:08
@aeisenberg
Copy link
Contributor Author

Would it make sense that we get this in and in a followup review I can clean up the query results? The query result values were designed to be extensible, so older extension versions can handle result types that it doesn't know about (just treats them as an "other error").

@aeisenberg
Copy link
Contributor Author

@alexet I am hoping to get this change in soon. Are my comments above sufficient for moving this ahead?

@alexet
Copy link
Contributor

alexet commented Oct 10, 2022

The problem is the new values don't match up, not just some different cases (e.g. 2 means something different). So we will report the wrong errors. I think we should keep it as legacy result and just use didRunSuccessfully ? QueryResultType.SUCCESS : QueryResultType.OTHER_ERROR. It will mean losing nice errors on downgrade but otherwise doesn't seem so bad. We could try mapping the errors better but the old messages don't match the errors that could occur very well anyway.

@aeisenberg
Copy link
Contributor Author

aeisenberg commented Oct 11, 2022

Oh...you're right. I just missed that. I'll do what you suggest.

We should still add versioning of the history, but I'll do that in another commit.

@alexet
Copy link
Contributor

alexet commented Oct 11, 2022

I don't think we care about the new results at all. The only reason we care about keeping the results si so we can serialise and deserialise legacy results. I think it is simpler with a patch luike the follwoing commit (based on 2 commit ago on this PR):5658030 . Then later we can make it optional and then even later we remove it. The eslint hook is currently crashing for me so that commit has no verification though.

@aeisenberg
Copy link
Contributor Author

That's fine. It's not exactly the way I would do it, but I'll use your commit in order to move this PR forward. And then clean up later.

@alexet
Copy link
Contributor

alexet commented Oct 12, 2022

I think my point is that the current code doesn't need the data as it is only there for compatibility so there is no point in changing the format.

@alexet alexet merged commit a071470 into main Oct 12, 2022
@alexet alexet deleted the aeisenberg/new-qs branch October 12, 2022 11:19
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.

2 participants