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

Serializing/deserializing methods for Event instances #251

Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
44f9bcc
`.fromJson` and `.toJson` added + move utils func
eliasyishak Mar 13, 2024
ce19f12
Use nullable static method to parse string json
eliasyishak Mar 13, 2024
9a55df9
Update event.dart
eliasyishak Mar 14, 2024
1846d97
Fix test
eliasyishak Mar 14, 2024
65c353d
Merge remote-tracking branch 'upstream/main' into 250-helper-method-t…
eliasyishak Mar 25, 2024
03dcdbe
Tests for encoding/decoding json
eliasyishak Mar 25, 2024
7a7511e
Store intersection in local var
eliasyishak Mar 26, 2024
9d2bd10
Remove exception thrown for nullable return
eliasyishak Mar 26, 2024
886b253
Use conditionals to check for any type errors
eliasyishak Mar 26, 2024
9df20b3
Test case added to check for invalid eventData
eliasyishak Mar 26, 2024
f16b37a
Update CHANGELOG.md
eliasyishak Mar 26, 2024
9602053
Refactor `Event.fromJson` to use pattern matching
eliasyishak Mar 27, 2024
2d58a3d
Use package:collection for comparing eventData
eliasyishak Mar 27, 2024
760d9b7
Use range for collection version
eliasyishak Mar 27, 2024
3977ae8
`fromLabel` static method renaming
eliasyishak Mar 27, 2024
d71b224
Fix test by refactoring unrelated DashTool static method
eliasyishak Mar 27, 2024
bd2fae1
Remove `when` clause and check inside if statement
eliasyishak Mar 28, 2024
7fd9071
`_deepCollectionEquality` to global scope + nit fix
eliasyishak Mar 29, 2024
8cacb0b
Remove collection dep + schema in dartdoc + nit fixes
eliasyishak Mar 29, 2024
1a85335
Add'l context to `Event.fromJson` static method
eliasyishak Mar 29, 2024
b412b59
Store intersection in local variable
eliasyishak Mar 29, 2024
e1d45d5
Refactor `DashTool.fromLabel`
eliasyishak Mar 29, 2024
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
1 change: 1 addition & 0 deletions pkgs/unified_analytics/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
- Get rid of `late` variables throughout implementation class, `AnalyticsImpl`
- Any error events (`Event.analyticsException`) encountered within package will be sent when invoking `Analytics.close`; replacing `ErrorHandler` functionality
- Exposing new method for `FakeAnalytics.sendPendingErrorEvents` to send error events on command
- Added `Event.fromJson` static method to generate instance of `Event` from JSON

## 5.8.8

Expand Down
10 changes: 10 additions & 0 deletions pkgs/unified_analytics/lib/src/enums.dart
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,16 @@ enum DashEvent {
required this.description,
this.toolOwner,
});

/// This takes in the string label for a given [DashEvent] and returns the
/// enum for that string label.
static DashEvent? getDashEventByLabel(String label) {
eliasyishak marked this conversation as resolved.
Show resolved Hide resolved
for (final event in DashEvent.values) {
if (event.label == label) return event;
}

return null;
eliasyishak marked this conversation as resolved.
Show resolved Hide resolved
}
}

/// Officially-supported clients of this package as logical
Expand Down
95 changes: 90 additions & 5 deletions pkgs/unified_analytics/lib/src/event.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import 'dart:convert';

import 'enums.dart';
import 'utils.dart';

final class Event {
final DashEvent eventName;
Expand Down Expand Up @@ -708,6 +707,10 @@ final class Event {
if (label != null) 'label': label,
};

/// Private constructor to be used when deserializing JSON into an instance
/// of [Event].
Event._({required this.eventName, required this.eventData});

@override
int get hashCode => Object.hash(eventName, jsonEncode(eventData));

Expand All @@ -716,11 +719,93 @@ final class Event {
other is Event &&
other.runtimeType == runtimeType &&
other.eventName == eventName &&
compareEventData(other.eventData, eventData);

@override
String toString() => jsonEncode({
_compareEventData(other.eventData, eventData);

/// Converts an instance of [Event] to JSON.
///
/// Example for [Event.timing] converted to JSON below.
/// ```json
/// {
/// "eventName": "timing",
/// "eventData": {
/// "workflow": "my-work-flow",
/// "variableName": "my-variable",
/// "elapsedMilliseconds": 123,
/// "label": "my-label"
/// }
/// }
/// ```
String toJson() => jsonEncode({
'eventName': eventName.label,
'eventData': eventData,
});

@override
String toString() => toJson();

/// Utility function to take in two maps [a] and [b] and compares them
/// to ensure that they have the same keys and values.
///
/// Used for the equality operator.
bool _compareEventData(Map<String, Object?> a, Map<String, Object?> b) {
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of creating our own, can we use something from package:collection? like DeepCollectionEquality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, this commit has it updated to use DeepCollectionEquality.equals to compare instead

@christopherfujino, i know we discussed trimming down dependencies for this package, do you think this is a valid reason to add another dep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following up on offline discussions with @christopherfujino, going to remove the dep on collection and use the same function for comparing two separate maps to reduce dependencies for this package

final keySetA = a.keys.toSet();
final keySetB = b.keys.toSet();
final intersection = keySetA.intersection(keySetB);

// Ensure that the keys are the same for each object
if (intersection.length != keySetA.length ||
intersection.length != keySetB.length) {
return false;
}

// Ensure that each of the key's values are the same
for (final key in a.keys) {
if (a[key] != b[key]) return false;
}

return true;
}

/// Returns a valid instance of [Event] if [json] follows the correct schema.
///
/// Schema is below with the following notes:
/// - The `eventName` key must have a string value that exists in [DashEvent].
/// - The `eventData` key must have a nested object as its value.
/// ```json
/// {
/// "eventName": "string-label",
/// "eventData": {
/// "myVar1": "value1",
/// "myVar2": 123
/// }
/// }
/// ```
eliasyishak marked this conversation as resolved.
Show resolved Hide resolved
static Event? fromJson(String json) {
try {
final jsonMap = jsonDecode(json) as Map<String, Object?>;

// Ensure the required keys are present
if (!jsonMap.containsKey('eventName') ||
!jsonMap.containsKey('eventData')) {
return null;
}

// Ensure the values for each key is the correct type
final eventName = jsonMap['eventName'];
final eventData = jsonMap['eventData'];
if (eventName is! String || eventData is! Map<String, Object?>) {
return null;
}

// Retrieve the correct DashEvent enum from the provided label
final dashEvent = DashEvent.getDashEventByLabel(eventName);
if (dashEvent == null) {
return null;
}

return Event._(eventName: dashEvent, eventData: eventData);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can simplify this using pattern matching:

if (jsonMap
    case  {
      'eventName': final String eventName, // you might be able to go further here to validate the DashEvent check in the pattern
      'eventData': final Map<String, Object?> eventData,
    }) {
  // Retrieve the correct DashEvent enum from the provided label
  final dashEvent = DashEvent.getDashEventByLabel(eventName);
  if (dashEvent == null) {
    return null;
  }
  return Event._(eventName: dashEvent, eventData: eventData);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh nice, this is a clever, had to catch up on Craigs pattern matching video to understand the syntax but it looks like this use case is highlighted pretty much exactly!

https://www.youtube.com/watch?v=aLvlqD4QS7Y&ab_channel=Flutter

Thank you for the suggestion! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Still digging through if this is possible) but i was able to simplify by adding a when clause to the case as the below

  static Event? fromJson(String json) {
    try {
      final jsonMap = jsonDecode(json) as Map<String, Object?>;

      // Ensure that eventName is a string and a valid label and
      // eventData is a nested object
      if (jsonMap
          case {
            'eventName': final String eventName,
            'eventData': final Map<String, Object?> eventData,
          } when DashEvent.getDashEventByLabel(eventName) != null) {
        return Event._(
          eventName: DashEvent.getDashEventByLabel(eventName)!,
          eventData: eventData,
        );
      }

      return null;
    } on FormatException {
      return null;
    }
  }

But it seems like i would have to make a call to DashEvent.getDashEventByLabel twice, do you know if its possible to assign this variable AND have it get checked within the when clause?

If it's not possible, i'll probably remove the when clause and leave the check within the body of the if

Copy link
Member

Choose a reason for hiding this comment

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

drive by comment--at least with this particular code, I think the solution is to pull DashEvent.getDashEventByLabel(eventName) into a local variable before the if statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't have access to that variable above the if statement though because it gets assigned in the if-case statement.. unless we don't check if it is a string first

Copy link
Member

Choose a reason for hiding this comment

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

ahh yeah, I see what you mean. carry on :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah I'm now sure if DashEvent.getDashEventByLabel(eventName) can be stored in a variable. Maybe @munificent knows?

Copy link
Member

Choose a reason for hiding this comment

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

There isn't always a great way to interleave a series of pattern matches and guards so that you can incrementally bind stuff after a guard passes. In this case, you could do:

if (jsonMap
    case {
      'eventName': final String eventName,
      'eventData': final Map<String, Object?> eventData,
    }) {
  if (DashEvent.getDashEventByLabel(eventName) case var event?) {
    return Event._(eventName: event, eventData: eventData);
  }
}

Whether you like seeing nested if-case statements like this is a matter of taste. :)

If you wanted to really jam everything into a pattern, you could make DashEvent.getDashEventByLabel an extension on String:

extension DashEventExtensions on String {
  // TODO: Whatever the real return type should be.
  Object? get dashEventForLabel => ...
}

You can call extension getters in object patterns, so that would let you do:

if (jsonMap
    case {
      'eventName': String(dashEventForLabel: var event?),
      'eventData': final Map<String, Object?> eventData,
    }) {
  return Event._(
    eventName: event,
    eventData: eventData,
  );
}

Then the pattern will only match if the map's 'eventName' key is mapped to a string where calling dashEventForLabel on it returns a non-null value. When that happens, the value is bound to event.

But this feels to me like twisting the language out of shape and I wouldn't do it (though it is kind of cool that you can).

Copy link
Member

Choose a reason for hiding this comment

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

We've discussed making case an expression instead of directly built into the if-case statement. If we did that, then you could probably chain them using && like:

if (jsonMap
    case {
      'eventName': final String eventName,
      'eventData': final Map<String, Object?> eventData,
    } &&
    DashEvent.getDashEventByLabel(eventName) case var event?) {
  return Event._(eventName: event, eventData: eventData);
}

But that's not in the language now and it's not clear if it's a good idea. It raises lots of tricky questions around the scope of the pattern variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@munificent yeah looks like your example in the second comment would be ideal but i can see how it get tricky.. i went with just having a separate conditional within the body of the if-case so it's easy to understand, thank you for the thorough comment!

} on FormatException {
return null;
}
}
}
20 changes: 0 additions & 20 deletions pkgs/unified_analytics/lib/src/utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -36,26 +36,6 @@ bool checkDirectoryForWritePermissions(Directory directory) {
return fileStat.modeString()[1] == 'w';
}

/// Utility function to take in two maps [a] and [b] and compares them
/// to ensure that they have the same keys and values
bool compareEventData(Map<String, Object?> a, Map<String, Object?> b) {
final keySetA = a.keys.toSet();
final keySetB = b.keys.toSet();

// Ensure that the keys are the same for each object
if (keySetA.intersection(keySetB).length != keySetA.length ||
keySetA.intersection(keySetB).length != keySetB.length) {
return false;
}

// Ensure that each of the key's values are the same
for (final key in a.keys) {
if (a[key] != b[key]) return false;
}

return true;
}

eliasyishak marked this conversation as resolved.
Show resolved Hide resolved
/// Format time as 'yyyy-MM-dd HH:mm:ss Z' where Z is the difference between the
/// timezone of t and UTC formatted according to RFC 822.
String formatDateTime(DateTime t) {
Expand Down
55 changes: 54 additions & 1 deletion pkgs/unified_analytics/test/event_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,9 @@ void main() {
test('Confirm all constructors were checked', () {
var constructorCount = 0;
for (var declaration in reflectClass(Event).declarations.keys) {
if (declaration.toString().contains('Event.')) constructorCount++;
// Count public constructors but omit private constructors
if (declaration.toString().contains('Event.') &&
!declaration.toString().contains('Event._')) constructorCount++;
}

// Change this integer below if your PR either adds or removes
Expand All @@ -568,4 +570,55 @@ void main() {
'`pkgs/unified_analytics/test/event_test.dart` '
'to reflect the changes made');
});

test('Serializing event to json successful', () {
final event = Event.analyticsException(
workflow: 'workflow',
error: 'error',
description: 'description',
);

final expectedResult = '{"eventName":"analytics_exception",'
'"eventData":{"workflow":"workflow",'
'"error":"error",'
'"description":"description"}}';

expect(event.toJson(), expectedResult);
});

test('Deserializing string to event successful', () {
final eventJson = '{"eventName":"analytics_exception",'
'"eventData":{"workflow":"workflow",'
'"error":"error",'
'"description":"description"}}';

final eventConstructed = Event.fromJson(eventJson);
expect(eventConstructed, isNotNull);
eventConstructed!;

expect(eventConstructed.eventName, DashEvent.analyticsException);
expect(eventConstructed.eventData, {
'workflow': 'workflow',
'error': 'error',
'description': 'description',
});
});

test('Deserializing string to event unsuccessful for invalid eventName', () {
final eventJson = '{"eventName":"NOT_VALID_NAME",'
'"eventData":{"workflow":"workflow",'
'"error":"error",'
'"description":"description"}}';

final eventConstructed = Event.fromJson(eventJson);
expect(eventConstructed, isNull);
});

test('Deserializing string to event unsuccessful for invalid eventData', () {
final eventJson = '{"eventName":"analytics_exception",'
'"eventData": "not_valid_event_data"}';

final eventConstructed = Event.fromJson(eventJson);
expect(eventConstructed, isNull);
});
}