Skip to content

urls [nfc]: Represent realm URL as Uri object, not String #68

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 4 commits into from
Apr 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 6 additions & 11 deletions lib/api/core.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ import 'package:flutter/foundation.dart';
import 'package:http/http.dart' as http;

class Auth {
const Auth({required this.realmUrl, required this.email, required this.apiKey});
Auth({required this.realmUrl, required this.email, required this.apiKey})
: assert(realmUrl.query.isEmpty && realmUrl.fragment.isEmpty);

final String realmUrl;
final Uri realmUrl;
final String email;
final String apiKey;
}
Expand Down Expand Up @@ -69,14 +70,8 @@ class LiveApiConnection extends ApiConnection {
@override
Future<String> get(String route, Map<String, dynamic>? params) async {
assert(_isOpen);
final baseUrl = Uri.parse(auth.realmUrl);
final url = Uri(
scheme: baseUrl.scheme,
userInfo: baseUrl.userInfo,
host: baseUrl.host,
port: baseUrl.port,
path: "/api/v1/$route",
queryParameters: encodeParameters(params));
final url = auth.realmUrl.replace(
path: "/api/v1/$route", queryParameters: encodeParameters(params));
Comment on lines +73 to +74
Copy link
Member

Choose a reason for hiding this comment

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

This version is implicitly assuming that realmUrl.fragment is empty.

Similarly the code in post and in the two other routes is assuming query and fragment are both empty. (In those, the old code was also assuming that, though differently so.)

That assumption is fine, but let's make it explicit with an assert. Can put it on the Auth constructor:

  Auth({required this.realmUrl, required this.email, required this.apiKey})
    : assert(realmUrl.query.isEmpty && realmUrl.fragment.isEmpty);

if (kDebugMode) print("GET $url");
final response = await _client.get(url, headers: _headers());
if (response.statusCode != 200) {
Expand All @@ -89,7 +84,7 @@ class LiveApiConnection extends ApiConnection {
Future<String> post(String route, Map<String, dynamic>? params) async {
assert(_isOpen);
final response = await _client.post(
Uri.parse("${auth.realmUrl}/api/v1/$route"),
auth.realmUrl.replace(path: "/api/v1/$route"),
headers: _headers(),
body: encodeParameters(params));
if (response.statusCode != 200) {
Expand Down
4 changes: 2 additions & 2 deletions lib/api/route/account.dart
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ part 'account.g.dart';

/// https://zulip.com/api/fetch-api-key
Future<FetchApiKeyResult> fetchApiKey({
required String realmUrl,
required Uri realmUrl,
required String username,
required String password,
}) async {
// TODO dedupe this part with LiveApiConnection; make this function testable
final response = await http.post(
Uri.parse("$realmUrl/api/v1/fetch_api_key"),
realmUrl.replace(path: "/api/v1/fetch_api_key"),
body: encodeParameters({
'username': RawParameter(username),
'password': RawParameter(password),
Expand Down
2 changes: 1 addition & 1 deletion lib/api/route/messages.dart
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ Future<SendMessageResult> sendMessage(
}) async {
// assert() is less verbose but would have no effect in production, I think:
// https://dart.dev/guides/language/language-tour#assert
if (Uri.parse(connection.auth.realmUrl).origin != 'https://chat.zulip.org') {
if (connection.auth.realmUrl.origin != 'https://chat.zulip.org') {
throw Exception('This binding can currently only be used on https://chat.zulip.org.');
}

Expand Down
6 changes: 3 additions & 3 deletions lib/api/route/realm.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ part 'realm.g.dart';
// See thread, and the zulip-mobile code and chat thread it links to:
// https://github.com/zulip/zulip-flutter/pull/55#discussion_r1160267577
Future<GetServerSettingsResult> getServerSettings({
required String realmUrl,
required Uri realmUrl,
}) async {
// TODO dedupe this part with LiveApiConnection; make this function testable
final response = await http.get(
Uri.parse("$realmUrl/api/v1/server_settings"));
realmUrl.replace(path: "/api/v1/server_settings"));
if (response.statusCode != 200) {
throw Exception('error on GET server_settings: status ${response.statusCode}');
}
Expand All @@ -45,7 +45,7 @@ class GetServerSettingsResult {

final bool emailAuthEnabled;
final bool requireEmailFormatUsernames;
final String realmUri;
final Uri realmUri;
final String realmName;
final String realmIcon;
final String realmDescription;
Expand Down
4 changes: 2 additions & 2 deletions lib/api/route/realm.g.dart

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions lib/model/store.dart
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ class PerAccountStore extends ChangeNotifier {

@immutable
class Account extends Auth {
const Account({
Account({
required super.realmUrl,
required super.email,
required super.apiKey,
Expand Down Expand Up @@ -243,8 +243,8 @@ class LiveGlobalStore extends GlobalStore {
///
/// See "Server credentials" in the project README for how to fill in the
/// `credential_fixture.dart` file this requires.
const Account _fixtureAccount = Account(
realmUrl: credentials.realmUrl,
final Account _fixtureAccount = Account(
realmUrl: Uri.parse(credentials.realmUrl),
email: credentials.email,
apiKey: credentials.apiKey,
userId: credentials.userId,
Expand Down
4 changes: 2 additions & 2 deletions lib/widgets/app.dart
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class ChooseAccountPage extends StatelessWidget {
for (final (:accountId, :account) in globalStore.accountEntries)
_buildAccountItem(context,
accountId: accountId,
title: Text(account.realmUrl),
title: Text(account.realmUrl.toString()),
subtitle: Text(account.email)),
const SizedBox(height: 12),
ElevatedButton(
Expand Down Expand Up @@ -103,7 +103,7 @@ class HomePage extends StatelessWidget {
const SizedBox(height: 12),
Text.rich(TextSpan(
text: 'Connected to: ',
children: [bold(store.account.realmUrl)])),
children: [bold(store.account.realmUrl.toString())])),
Text.rich(TextSpan(
text: 'Zulip server version: ',
children: [bold(store.zulipVersion)])),
Expand Down
4 changes: 2 additions & 2 deletions lib/widgets/content.dart
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,7 @@ class RealmContentNetworkImage extends StatelessWidget {
isAntiAlias: isAntiAlias,

// Only send the auth header to the server `auth` belongs to.
headers: parsedSrc.origin == Uri.parse(auth.realmUrl).origin
headers: parsedSrc.origin == auth.realmUrl.origin
? authHeader(auth)
: null,

Expand All @@ -609,7 +609,7 @@ class RealmContentNetworkImage extends StatelessWidget {
// This may dissolve when we start passing around URLs as [Uri] objects instead
// of strings.
String resolveUrl(String url, Account account) {
final realmUrl = Uri.parse(account.realmUrl); // TODO clean this up
final realmUrl = account.realmUrl;
final resolved = realmUrl.resolve(url); // TODO handle if fails to parse
return resolved.toString();
}
Expand Down
8 changes: 4 additions & 4 deletions lib/widgets/login.dart
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class _AddAccountPageState extends State<AddAccountPage> {
}

// TODO(#35): show feedback that we're working, while fetching server settings
final serverSettings = await getServerSettings(realmUrl: url.toString());
final serverSettings = await getServerSettings(realmUrl: url);
if (context.mounted) {} // https://github.com/dart-lang/linter/issues/4007
else {
return;
Expand Down Expand Up @@ -105,7 +105,7 @@ class _EmailPasswordLoginPageState extends State<EmailPasswordLoginPage> {
Future<int> _getUserId(FetchApiKeyResult fetchApiKeyResult) async {
final FetchApiKeyResult(:email, :apiKey) = fetchApiKeyResult;
final auth = Auth(
realmUrl: widget.realmUrl.toString(), email: email, apiKey: apiKey);
realmUrl: widget.realmUrl, email: email, apiKey: apiKey);
final connection = LiveApiConnection(auth: auth); // TODO make this widget testable
return (await getOwnUser(connection)).userId;
}
Expand All @@ -124,7 +124,7 @@ class _EmailPasswordLoginPageState extends State<EmailPasswordLoginPage> {
final FetchApiKeyResult result;
try {
result = await fetchApiKey(
realmUrl: realmUrl.toString(), username: email, password: password);
realmUrl: realmUrl, username: email, password: password);
} on Exception catch (e) { // TODO(#37): distinguish API exceptions
// TODO(#35): give feedback to user on failed login
debugPrint(e.toString());
Expand All @@ -139,7 +139,7 @@ class _EmailPasswordLoginPageState extends State<EmailPasswordLoginPage> {
}

final account = Account(
realmUrl: realmUrl.toString(),
realmUrl: realmUrl,
email: result.email,
apiKey: result.apiKey,
userId: userId,
Expand Down
2 changes: 1 addition & 1 deletion test/api/fake_api.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import 'package:zulip/model/store.dart';

/// An [ApiConnection] that accepts and replays canned responses, for testing.
class FakeApiConnection extends ApiConnection {
FakeApiConnection({required String realmUrl, required String email})
FakeApiConnection({required Uri realmUrl, required String email})
: super(auth: Auth(
realmUrl: realmUrl, email: email, apiKey: _fakeApiKey));

Expand Down
4 changes: 2 additions & 2 deletions test/api/route/messages_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@ import 'route_checks.dart';
void main() {
test('sendMessage accepts fixture realm', () async {
final connection = FakeApiConnection(
realmUrl: 'https://chat.zulip.org/', email: '[email protected]');
realmUrl: Uri.parse('https://chat.zulip.org/'), email: '[email protected]');
connection.prepare(jsonEncode(SendMessageResult(id: 42).toJson()));
check(sendMessage(connection, content: 'hello', topic: 'world'))
.completes(it()..id.equals(42));
});

test('sendMessage rejects unexpected realm', () async {
final connection = FakeApiConnection(
realmUrl: 'https://chat.example/', email: '[email protected]');
realmUrl: Uri.parse('https://chat.example/'), email: '[email protected]');
connection.prepare(jsonEncode(SendMessageResult(id: 42).toJson()));
check(sendMessage(connection, content: 'hello', topic: 'world'))
.throws();
Expand Down
6 changes: 3 additions & 3 deletions test/example_data.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ import 'package:zulip/api/model/initial_snapshot.dart';
import 'package:zulip/api/model/model.dart';
import 'package:zulip/model/store.dart';

const String realmUrl = 'https://chat.example/';
final Uri realmUrl = Uri.parse('https://chat.example/');

const String recentZulipVersion = '6.1';
const int recentZulipFeatureLevel = 164;

const Account selfAccount = Account(
final Account selfAccount = Account(
realmUrl: realmUrl,
email: 'self@example',
apiKey: 'asdfqwer',
Expand All @@ -17,7 +17,7 @@ const Account selfAccount = Account(
zulipMergeBase: recentZulipVersion,
);

const Account otherAccount = Account(
final Account otherAccount = Account(
realmUrl: realmUrl,
email: 'other@example',
apiKey: 'sdfgwert',
Expand Down