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

maintained RpcReqError with headers in vkext #915

Merged

Conversation

DrDet
Copy link
Contributor

@DrDet DrDet commented Oct 12, 2023

Context

TL error serialization is historically totally broken and doesn't fit the scheme.
By according to TL scheme it must be like this:

reqError#b527877d {X:Type} error_code:int error:string = ReqResult X;

But actually it comes as:

rpcReqError#7ae432f5 {X:Type} query_id:long error_code:int error:string = RpcReqResult X;

rpcReqError#7ae432f5 is outer error, but it's fetched as inner one instead of reqError#b527877d, while latter is never used at all.

Problem

This weird deserialization is done via workaround as tl_fetch_lookup right before the answer fetching. And it's done inconsistently: if we require extra response headers via:

rpcInvokeReqExtra#f3ef81a9 {flags:#}

we will get the error on parsing them in case of the error, although in case of normal response everything is fine.

Changes

Now we can successfully fetch error with extra fields (header), if we require them.
In typed mode it's fetched as rpcResponseHeader object, containing rpcResponseError in its $result field.
It doesn't fit php-doc types, but it's fair, because it doesn't fit TL scheme at all.
This approach seems for me the best option, as it helps to avoid extra related workarounds in tl2php, tlo-parser, kphp, etc.
In kphp headers will just be skipped and rpcResponseError will be returned as usual.


In the future this workaround will be fixed, and serialization will fit the TL scheme and work correctly in kphp and vkext the same way.

@DrDet DrDet self-assigned this Oct 12, 2023
@DrDet DrDet force-pushed the dvaksman/vkext/maintain-rpc-req-error-with-header-lookup branch 2 times, most recently from 29a6ef1 to 176c8f5 Compare October 13, 2023 17:17
Copy link

@KokorinIlya KokorinIlya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Парсинг выглядит ок

@DrDet DrDet force-pushed the dvaksman/vkext/maintain-rpc-req-error-with-header-lookup branch 2 times, most recently from f5e92de to 88d28fd Compare October 19, 2023 16:18
@DrDet DrDet changed the title WIP: maintained RpcReqError with header in vkext maintained RpcReqError with headers in vkext Oct 19, 2023
Hidanio
Hidanio previously approved these changes Oct 24, 2023
troy4eg
troy4eg previously approved these changes Oct 24, 2023
vkext/vkext-rpc-tl-serialization.cpp Show resolved Hide resolved
@DrDet DrDet dismissed stale reviews from troy4eg and Hidanio via 6a9f62b October 24, 2023 16:48
@DrDet DrDet merged commit 126a015 into master Oct 24, 2023
5 checks passed
@DrDet DrDet deleted the dvaksman/vkext/maintain-rpc-req-error-with-header-lookup branch October 24, 2023 16:53
@Danil42Russia Danil42Russia modified the milestones: next, 19.10.2023 Nov 2, 2023
@Danil42Russia Danil42Russia added enhancement New feature or request runtime Feature related to runtime labels Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request runtime Feature related to runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants