Skip to content

api: Carry zulipFeatureLevel on ApiConnection, and use it for sending "direct" and "dm" #159

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.

Sign up for GitHub

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 7 commits into from
Jun 8, 2023
Merged

api: Carry zulipFeatureLevel on ApiConnection, and use it for sending "direct" and "dm" #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
81faeb3
api test: Fix with_ to await callback first, then close
gnprice Jun 6, 2023
1aacda4
api test [nfc]: Factor out common checks in sendMessage tests
gnprice Jun 6, 2023
d53f3fb
api test: Add getMessages tests
gnprice Jun 6, 2023
e70e13d
store [nfc]: Add todo on keeping zulipVersion updated
gnprice Jun 5, 2023
e3a3496
api [nfc]: Carry zulipFeatureLevel on ApiConnection
gnprice Jun 3, 2023
fd54070
api: In sendMessage, use "direct" where possible
gnprice Jun 5, 2023
3e797d4
api: In getMessages, use "dm" operator where possible
gnprice Jun 5, 2023
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
25 changes: 23 additions & 2 deletions lib/api/core.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class ApiConnection {
/// For talking to a live server, use [ApiConnection.live].
ApiConnection({
required this.realmUrl,
required this.zulipFeatureLevel, // required even though nullable; see field doc
String? email,
String? apiKey,
required http.Client client,
Expand All @@ -34,11 +35,31 @@ class ApiConnection {
_client = client;

/// Construct an API connection that talks to a live Zulip server over the real network.
ApiConnection.live({required Uri realmUrl, String? email, String? apiKey})
: this(realmUrl: realmUrl, email: email, apiKey: apiKey, client: http.Client());
ApiConnection.live({
required Uri realmUrl,
required int? zulipFeatureLevel, // required even though nullable; see field doc
String? email,
String? apiKey,
}) : this(client: http.Client(),
realmUrl: realmUrl, zulipFeatureLevel: zulipFeatureLevel,
email: email, apiKey: apiKey);

final Uri realmUrl;

/// The server's last known Zulip feature level, if any.
///
/// Individual route/endpoint bindings may use this to adapt
/// for compatibility with older servers.
///
/// If this is null, this [ApiConnection] may be used only for the
/// [getServerSettings] route. Calls to other routes may throw an exception.
/// Constructors therefore require this as a parameter, so that a null value
/// must be passed explicitly.
///
/// See:
/// * API docs at <https://zulip.com/api/changelog>.
int? zulipFeatureLevel;

final String? _authValue;

void addAuth(http.BaseRequest request) {
Expand Down
48 changes: 43 additions & 5 deletions lib/api/model/narrow.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,53 @@ class ApiNarrowTopic extends ApiNarrowElement {
}

/// An [ApiNarrowElement] with the 'dm', or legacy 'pm-with', operator.
///
/// An instance directly of this class must not be serialized with [jsonEncode],
/// and more generally its [operator] getter must not be called.
/// Instead, call [resolve] and use the object it returns.
class ApiNarrowDm extends ApiNarrowElement {
@override String get operator => 'pm-with'; // TODO(#146): use 'dm' where possible
@override String get operator {
Copy link

Choose a reason for hiding this comment

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

This sort of thing was why I was asking about the dart format. I see both this style and the more traditional style throughout the codebase.

@override
String get operator {}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, for the classes in this file (before this PR, anyway), they're all so short and simple and parallel to each other that I felt the one-line form like

  @override String get operator => 'topic';

was helpful for reading, by helping make the parallelism easy to see.

If we did adopt automated formatting, though, I wouldn't really mind these going into multi-line form as a consequence of that.

At a quick grep (git grep '@override '), I'm only finding one other spot in the codebase where we use the one-line form:

class UriConverter extends TypeConverter<Uri, String> {
  const UriConverter();
  @override String toSql(Uri value) => value.toString();
  @override Uri fromSql(String fromDb) => Uri.parse(fromDb);
}

The idea there is similar, and again it's not something I'd particularly regret going away if we adopted auto-formatting.

assert(false,
"The [operator] getter was called on a plain [ApiNarrowDm]. "
"Before passing to [jsonEncode] or otherwise getting [operator], "
"the [ApiNarrowDm] must be replaced by the result of [ApiNarrowDm.resolve]."
);
return "dm";
}

@override final List<int> operand;

ApiNarrowDm(this.operand, {super.negated});

factory ApiNarrowDm.fromJson(Map<String, dynamic> json) => ApiNarrowDm(
(json['operand'] as List<dynamic>).map((e) => e as int).toList(),
negated: json['negated'] as bool? ?? false,
);
factory ApiNarrowDm.fromJson(Map<String, dynamic> json) {
var operand = (json['operand'] as List<dynamic>).map((e) => e as int).toList();
var negated = json['negated'] as bool? ?? false;
return (json['operator'] == 'pm-with')
? ApiNarrowPmWith._(operand, negated: negated)
: ApiNarrowDmModern._(operand, negated: negated);
}

/// This element resolved, as either an [ApiNarrowDmModern] or an [ApiNarrowPmWith].
ApiNarrowDm resolve({required bool legacy}) {
return legacy ? ApiNarrowPmWith._(operand, negated: negated)
: ApiNarrowDmModern._(operand, negated: negated);
}
}

/// An [ApiNarrowElement] with the 'dm' operator (and not the legacy 'pm-with').
///
/// To construct one of these, use [ApiNarrowDm.resolve].
class ApiNarrowDmModern extends ApiNarrowDm {
@override String get operator => 'dm';

ApiNarrowDmModern._(super.operand, {super.negated});
}

/// An [ApiNarrowElement] with the legacy 'pm-with' operator.
///
/// To construct one of these, use [ApiNarrowDm.resolve].
class ApiNarrowPmWith extends ApiNarrowDm {
@override String get operator => 'pm-with';

ApiNarrowPmWith._(super.operand, {super.negated});
}
3 changes: 2 additions & 1 deletion lib/api/route/account.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ part 'account.g.dart';
/// https://zulip.com/api/fetch-api-key
Future<FetchApiKeyResult> fetchApiKey({
required Uri realmUrl,
required int? zulipFeatureLevel,
required String username,
required String password,
}) async {
// TODO make this function testable by taking ApiConnection from caller
final connection = ApiConnection.live(realmUrl: realmUrl);
final connection = ApiConnection.live(realmUrl: realmUrl, zulipFeatureLevel: zulipFeatureLevel);
try {
return await connection.post('fetchApiKey', FetchApiKeyResult.fromJson, 'fetch_api_key', {
'username': RawParameter(username),
Expand Down
10 changes: 9 additions & 1 deletion lib/api/route/messages.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ Future<GetMessagesResult> getMessages(ApiConnection connection, {
bool? applyMarkdown,
// bool? useFirstUnreadAnchor // omitted because deprecated
}) {
if (narrow.any((element) => element is ApiNarrowDm)) {
final supportsOperatorDm = connection.zulipFeatureLevel! >= 177; // TODO(server-7)
narrow = narrow.map((element) => switch (element) {
ApiNarrowDm() => element.resolve(legacy: !supportsOperatorDm),
_ => element,
}).toList();
}
return connection.get('getMessages', GetMessagesResult.fromJson, 'messages', {
'narrow': narrow,
'anchor': switch (anchor) {
Expand Down Expand Up @@ -99,13 +106,14 @@ Future<SendMessageResult> sendMessage(
String? queueId,
String? localId,
}) {
final supportsTypeDirect = connection.zulipFeatureLevel! >= 174; // TODO(server-7)
return connection.post('sendMessage', SendMessageResult.fromJson, 'messages', {
if (destination is StreamDestination) ...{
'type': RawParameter('stream'),
'to': destination.streamId,
'topic': RawParameter(destination.topic),
} else if (destination is DmDestination) ...{
'type': RawParameter('private'), // TODO(#146): use 'direct' where possible
'type': supportsTypeDirect ? RawParameter('direct') : RawParameter('private'),
'to': destination.userIds,
} else ...(
throw Exception('impossible destination') // TODO(dart-3) show this statically
Expand Down
3 changes: 2 additions & 1 deletion lib/api/route/realm.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,10 @@ part 'realm.g.dart';
// https://github.com/zulip/zulip-flutter/pull/55#discussion_r1160267577
Future<GetServerSettingsResult> getServerSettings({
required Uri realmUrl,
required int? zulipFeatureLevel,
}) async {
// TODO make this function testable by taking ApiConnection from caller
final connection = ApiConnection.live(realmUrl: realmUrl);
final connection = ApiConnection.live(realmUrl: realmUrl, zulipFeatureLevel: zulipFeatureLevel);
try {
return await connection.get('getServerSettings', GetServerSettingsResult.fromJson, 'server_settings', null);
} finally {
Expand Down
7 changes: 4 additions & 3 deletions lib/model/store.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,10 @@ class PerAccountStore extends ChangeNotifier {
maxFileUploadSizeMib = initialSnapshot.maxFileUploadSizeMib;

final Account account;
final ApiConnection connection;
final ApiConnection connection; // TODO(#135): update zulipFeatureLevel with events

// TODO(#135): Keep all this data updated by handling Zulip events from the server.
final String zulipVersion;
final String zulipVersion; // TODO get from account; update there on initial snapshot
final Map<int, User> users;
final Map<int, ZulipStream> streams;
final Map<int, Subscription> subscriptions;
Expand Down Expand Up @@ -344,7 +344,8 @@ class LivePerAccountStore extends PerAccountStore {
/// In the future this might load an old snapshot from local storage first.
static Future<PerAccountStore> load(Account account) async {
final connection = ApiConnection.live(
realmUrl: account.realmUrl, email: account.email, apiKey: account.apiKey);
realmUrl: account.realmUrl, zulipFeatureLevel: account.zulipFeatureLevel,
email: account.email, apiKey: account.apiKey);

final stopwatch = Stopwatch()..start();
final initialSnapshot = await registerQueue(connection); // TODO retry
Expand Down
10 changes: 7 additions & 3 deletions lib/widgets/login.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ class _AddAccountPageState extends State<AddAccountPage> {
try {
final GetServerSettingsResult serverSettings;
try {
serverSettings = await getServerSettings(realmUrl: url!);
serverSettings = await getServerSettings(realmUrl: url!, zulipFeatureLevel: null);
} catch (e) {
if (!context.mounted) {
return;
Expand Down Expand Up @@ -255,7 +255,9 @@ class _PasswordLoginPageState extends State<PasswordLoginPage> {
Future<int> _getUserId(FetchApiKeyResult fetchApiKeyResult) async {
final FetchApiKeyResult(:email, :apiKey) = fetchApiKeyResult;
final connection = ApiConnection.live( // TODO make this widget testable
realmUrl: widget.serverSettings.realmUri, email: email, apiKey: apiKey);
realmUrl: widget.serverSettings.realmUri,
zulipFeatureLevel: widget.serverSettings.zulipFeatureLevel,
email: email, apiKey: apiKey);
return (await getOwnUser(connection)).userId;
}

Expand All @@ -279,7 +281,9 @@ class _PasswordLoginPageState extends State<PasswordLoginPage> {
final FetchApiKeyResult result;
try {
result = await fetchApiKey(
realmUrl: realmUrl, username: username, password: password);
realmUrl: realmUrl,
zulipFeatureLevel: widget.serverSettings.zulipFeatureLevel,
username: username, password: password);
} on ApiRequestException catch (e) {
if (!context.mounted) return;
// TODO(#105) give more helpful feedback. The RN app is
Expand Down
29 changes: 24 additions & 5 deletions test/api/fake_api.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,18 +84,30 @@ class FakeHttpClient extends http.BaseClient {

/// An [ApiConnection] that accepts and replays canned responses, for testing.
class FakeApiConnection extends ApiConnection {
FakeApiConnection({Uri? realmUrl})
: this._(realmUrl: realmUrl ?? eg.realmUrl, client: FakeHttpClient());
/// Construct an [ApiConnection] that accepts and replays canned responses, for testing.
///
/// If `zulipFeatureLevel` is omitted, it defaults to [eg.futureZulipFeatureLevel],
/// which causes route bindings to behave as they would for the
/// latest Zulip server versions.
/// To set `zulipFeatureLevel` to null, pass null explicitly.
FakeApiConnection({Uri? realmUrl, int? zulipFeatureLevel = eg.futureZulipFeatureLevel})
: this._(
realmUrl: realmUrl ?? eg.realmUrl,
zulipFeatureLevel: zulipFeatureLevel,
client: FakeHttpClient(),
);

FakeApiConnection.fromAccount(Account account)
: this._(
realmUrl: account.realmUrl,
zulipFeatureLevel: account.zulipFeatureLevel,
email: account.email,
apiKey: account.apiKey,
client: FakeHttpClient());

FakeApiConnection._({
required super.realmUrl,
required super.zulipFeatureLevel,
super.email,
super.apiKey,
required this.client,
Expand All @@ -105,17 +117,24 @@ class FakeApiConnection extends ApiConnection {

/// Run the given callback on a fresh [FakeApiConnection], then close it,
/// using try/finally.
///
/// If `zulipFeatureLevel` is omitted, it defaults to [eg.futureZulipFeatureLevel],
/// which causes route bindings to behave as they would for the
/// latest Zulip server versions.
/// To set `zulipFeatureLevel` to null, pass null explicitly.
static Future<T> with_<T>(
Future<T> Function(FakeApiConnection connection) fn, {
Uri? realmUrl,
int? zulipFeatureLevel = eg.futureZulipFeatureLevel,
Account? account,
}) async {
assert((account == null) || (realmUrl == null));
assert((account == null)
|| (realmUrl == null && zulipFeatureLevel == eg.futureZulipFeatureLevel));
final connection = (account != null)
? FakeApiConnection.fromAccount(account)
: FakeApiConnection(realmUrl: realmUrl);
: FakeApiConnection(realmUrl: realmUrl, zulipFeatureLevel: zulipFeatureLevel);
try {
return fn(connection);
return await fn(connection);
Copy link
Collaborator

Choose a reason for hiding this comment

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

api test: Fix with_ to await callback first, then close

As is, the `finally` block gets run before the future returned by
the callback has completed.  I believe the underlying issue here
is this one in the Dart SDK:
  https://github.com/dart-lang/sdk/issues/44395

The fix is to await the future and return the result of that,
rather than returning the future directly.

The Dart language folks (or at least some of them) hope to
eventually deal with this by making the old code here a type error,
requiring the value passed to `return` to have type T rather
than Future<T>:
  https://github.com/dart-lang/language/issues/870

Interesting; yeah. I think there's a "gotcha" like this in JavaScript, too.

} finally {
connection.close();
}
Expand Down
Loading