-
Notifications
You must be signed in to change notification settings - Fork 202
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
Conversation
7698519
to
176fac0
Compare
ab25ea1
to
e5fe214
Compare
7877cdf
to
db0fbaa
Compare
Ensures better backwards compatibility with legacy query objects.
2a50c4d
to
46ce536
Compare
@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. |
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? |
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. |
3046e90
to
d461372
Compare
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. |
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
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 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. |
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"). |
@alexet I am hoping to get this change in soon. Are my comments above sufficient for moving this ahead? |
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 |
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. |
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. |
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. |
3fdb70a
to
b95af11
Compare
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. |
This is a new variant of #1499 that is based on my most recent changes to #1498.