-
Notifications
You must be signed in to change notification settings - Fork 5
Implemented a way to get a valid x-client-transaction-id header for requests #64
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
@@ -0,0 +1,6 @@ | |||
import 'package:quacker/database/repository.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was moved from client_regular_account.dart
I finally decided to remove the generated files from the commit to ease the review :) Tell me if you really need these for the release |
Please find the build here if you want to test this fix: https://github.com/Teskann/Quacker/releases/tag/untagged-44319a22a7b1fab4581e (I kept the same package name that I signed with my own key) |
Your a lifesaver, I'll take a look at the code later today :) |
|
||
if (authHeader != null) { | ||
return await model.fetch(uri, headers: headers, log: log, prefs: prefs, authHeader: authHeader); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefs
was unused, so I removed it
'authorization': bearerToken, | ||
'x-twitter-active-user': 'yes', | ||
'user-agent': userAgentHeader.toString() | ||
...baseHeaders |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
baseHeaders
now contains everything that was here before so it can safely replace what was previously declared. It will include more headers than before however. Tell me if this is a problem. See headers.dart
for more info
}); | ||
|
||
return response; | ||
} | ||
|
||
Future<List<Map<String, Object?>>> getAccounts() async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to accounts.dart
Future<void> deleteAccount(String username) async { | ||
var database = await Repository.writable(); | ||
database.delete(tableAccounts, where: 'id = ?', whereArgs: [username]); | ||
} | ||
|
||
Future<Map<dynamic, dynamic>?> getAuthHeader(BasePrefService prefs) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getAuthHeader
was moved to headers.dart
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefs argument was unused so I removed it
"@x_client_transaction_id_provider": {}, | ||
"x_client_transaction_id_provider_description": "Set the x-client-transaction-id provider. It must he a domain name, without http/https prefix. For more information about it, check https://github.com/Teskann/x-client-transaction-id-generator", | ||
"@x_client_transaction_id_provider_description": {} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tell me if these strings look clear to you.
I could use an LLM to translate them to all languages Quacker supports. I didn't because I don't know what you think about it for this project. Tell me if you want me to do it
@@ -0,0 +1,6 @@ | |||
import 'package:quacker/database/repository.dart'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before you start the review, let me precise this is the first time I work with Dart and a Flutter application. I'm a full time C++ developer. I don't know the best practices for Dart. I tried to mimicate the rest of the code but don't hesitate to ask me to do some corrections if needed :)
throw Exception('Failed to get x-client-transaction-id. Status code: ${response.statusCode}'); | ||
} | ||
} catch (e) { | ||
throw Exception('Error getting x-client-transaction-id: $e'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exception will make it easy to know that x-client-transaction-id.xyz does not work anymore with this message for instance if the server is down.
If the transaction id is correctly generated but not valid on X's side, we will get the same 404 error as before. If 404 happen again in the future, we should consider taking a close look at the validity of the x-client-transaction-id
and maybe we will have to update the server that provides it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I'm the owner of the server & domain, I didn't want to block the app if any of these is down/sold. This is why I added an option to change the URL of the provider in the settings. Anybody could create its own instance. Of course in this case you should update the default URL in Quacker, but users would have a way to change it themselves without any app update
My pleasure to help you on this project. It's amazing, I used it everyday when it worked. It's the best Fritter fork out there. One thing about why I decided to rely on an external server to generate the This is why I decided not to "transalte" the Python implementation to Dart, even if it would have probably worked. This will reduce our maintainance and we won't have to manually reimplement every single commit that is pushed to the Python implementation. Just update the package version that the server uses and that's it. I added several comments in the code to ease your review. |
Would it be possible to have the app start its own copy of the server locally, to avoid depending on the uptime of a third-party server? |
Adressed #58
Issue was a missing header in the X graphql requests:
x-client-transaction-id
This ID is supposed to be generated. As the generation is quite complex, I decided to rely on an existing working implementation: https://github.com/iSarabjitDhiman/XClientTransaction
This implementation is in Python but is very light weight. This is why I created a flask webserver that runs iSarabjitDhiman/XClientTransaction.
The project wrapping it to flask is available here: https://github.com/Teskann/x-client-transaction-id-generator
I created a public instance of this server on https://x-client-transaction-id-generator.xyz/
The idea in Quacker is to ask this server to generate an
x-client-transaction-id
for us. As this operation is very fast, I don't think it's an issue to contact another server.I added an option to change the URL of the provider so that users can use any other working instance, maybe their own.
Regarding the code:
xclient-transaction-id
in General settings