fix: do not modify custom attributes casing #234

levibostian · 2022-12-08T05:36:07Z

Closes: https://github.com/customerio/issues/issues/8724
Closes: #223

In this PR:

I created suite of integration tests that view the raw JSON sent to our API to see if the SDK modifies the custom attributes in any step along the way.
Modified the JsonAdapter to no longer modify JSON keys casing at all. It is convenient for us to do this, but it could create this bug again in the future. Instead, Swift objects that want to use snake_case JSON key must implement CodingKeys in the struct (you will see examples in this PR).

QA tests:

Test backwards compatibility with Queue storage since Queue saves data into JSON.

Turn iPhone to airplane mode.
Install version 1.21.2 of RH. Do lots of random activities in the app so the Queue adds different types of events to the queue.
Install version of RH that tests this PR.
Do more random activities in the app to add more events to the Queue.
Turn off iPhone airplane mode.
Wait and see if all events get sent to Fly.

Complete each step to get your pull request merged in. Learn more about the workflow this project uses.

Assign members of your team to review the pull request.
Wait for pull request status checks to complete. If there are problems, fix them until you see that all status checks are passing.
Wait until the pull request has been reviewed and approved by a teammate
After pull request is approved, and you determine it's ready add the label "Ready to merge" to the pull request. A bot will squash and merge the pull request for you after the label is added.

ami-oss-ci · 2022-12-08T05:36:20Z

Pull request title looks good 👍!

If this pull request gets merged, it will cause a new release of the software. Example: If this project's latest release version is 1.0.0. If this pull request gets merged in, the next release of this project will be 1.0.1. This pull request is not a breaking change.

All merged pull requests will eventually get deployed. But some types of pull requests will trigger a deployment (such as features and bug fixes) while some pull requests will wait to get deployed until a later time.

To merge this pull request, add the label Ready to merge to this pull request and I'll merge it for you.

This project uses a special format for pull requests titles. Expand this section to learn more (expand by clicking the ᐅ symbol on the left side of this sentence)...

This project uses a special format for pull requests titles. Don't worry, it's easy!

This pull request title should be in this format:

<type>: short description of change being made

If your pull request introduces breaking changes to the code, use this format:

<type>!: short description of breaking change

where <type> is one of the following:

feat: - A feature is being added or modified by this pull request. Use this if you made any changes to any of the features of the project.
fix: - A bug is being fixed by this pull request. Use this if you made any fixes to bugs in the project.
docs: - This pull request is making documentation changes, only.
refactor: - A change was made that doesn't fix a bug or add a feature.
test: - Adds missing tests or fixes broken tests.
style: - Changes that do not effect the code (whitespace, linting, formatting, semi-colons, etc)
perf: - Changes improve performance of the code.
build: - Changes to the build system (maven, npm, gulp, etc)
ci: - Changes to the CI build system (Travis, GitHub Actions, Circle, etc)
chore: - Other changes to project that don't modify source code or test files.
revert: - Reverts a previous commit that was made.

Examples:

feat: edit profile photo
refactor!: remove deprecated v1 endpoints
build: update npm dependencies
style: run formatter

Need more examples? Want to learn more about this format? Check out the official docs.

Note: If your pull request does multiple things such as adding a feature and makes changes to the CI server and fixes some bugs then you might want to consider splitting this pull request up into multiple smaller pull requests.

codecov · 2022-12-08T05:37:14Z

Codecov Report

Merging #234 (818e4f4) into develop (455a6a1) will increase coverage by 1.26%.
The diff coverage is 75.00%.

@@             Coverage Diff             @@
##           develop     #234      +/-   ##
===========================================
+ Coverage    51.03%   52.30%   +1.26%     
===========================================
  Files          100      100              
  Lines         1107     1107              
===========================================
+ Hits           565      579      +14     
+ Misses         542      528      -14

Impacted Files	Coverage Δ
...s/Common/Background Queue/ApiSyncQueueRunner.swift	`83.33% <ø> (ø)`
Sources/Common/Background Queue/Queue.swift	`64.28% <ø> (ø)`
Sources/Common/Background Queue/QueueStorage.swift	`93.75% <0.00%> (ø)`
...urces/Common/Background Queue/Type/QueueTask.swift	`100.00% <ø> (ø)`
...mmon/Background Queue/Type/QueueTaskMetadata.swift	`100.00% <ø> (ø)`
Sources/Common/Service/HttpClient.swift	`92.85% <ø> (ø)`
...es/MessagingPush/CustomerIOParsedPushPayload.swift	`100.00% <ø> (ø)`
Sources/Tracking/CustomerIOImplementation.swift	`80.76% <ø> (+11.53%)`	⬆️
...urces/Tracking/Service/Request/MetricRequest.swift	`0.00% <ø> (-100.00%)`	⬇️
.../Common/autogenerated/AutoMockable.generated.swift	`43.47% <50.00%> (+0.04%)`	⬆️
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

levibostian · 2022-12-08T15:38:47Z

Since we changed the JsonAdapter, this probably breaks backwards compitability with queue storage.

If a customer has items in the Queue using SDK v1 and then runs v2, I imagine the app would crash because the JsonAdapter no longer can read the old v1 queue data.

The fix is to:

Create test that tests backwards compatibility.
Modify CodingKeys of Queue tasks.

levibostian · 2022-12-08T16:24:25Z

Besides, this use case before merging I need to:

Find all the places where jsonAdapter is being used in the code to see if I can find a use case I have not thought about yet.

ami-oss-ci · 2022-12-08T19:14:31Z

	Warnings
⚠️	Swift Package Manager files (Package.) were modified but Cocoapods files (.podspec) files were not. This is error-prone when updating dependencies in one service but not the other. Double-check that you updated all of the correct files.

Generated by 🚫 dangerJS against 667bad3

ami-oss-ci · 2022-12-08T19:22:14Z

	Warnings
⚠️	Swift Package Manager files (Package.) were modified but Cocoapods files (.podspec) files were not. This is error-prone when updating dependencies in one service but not the other. Double-check that you updated all of the correct files.

Generated by 🚫 dangerJS against df7c3b7

levibostian · 2022-12-08T19:30:53Z

Fixed use case mentioned before

ami-oss-ci · 2022-12-08T19:31:28Z

	Warnings
⚠️	Swift Package Manager files (Package.) were modified but Cocoapods files (.podspec) files were not. This is error-prone when updating dependencies in one service but not the other. Double-check that you updated all of the correct files.

Generated by 🚫 dangerJS against f1ab7f4

ami-oss-ci · 2022-12-09T04:34:35Z

	Warnings
⚠️	Swift Package Manager files (Package.) were modified but Cocoapods files (.podspec) files were not. This is error-prone when updating dependencies in one service but not the other. Double-check that you updated all of the correct files.

Generated by 🚫 dangerJS against 818e4f4

ami-aman · 2022-12-09T11:49:32Z

Tests/Shared/SampleData/CustomAttributesSampleData.swift

+        "last_name": "Green",
+        "HOBBY": "football",
+        "nested": [
+            "is adult": true,


Is the space in is adult intentional ?

Yes. I wanted to test this scenario in case a customer wanted to send us an attribute with a space in it.

ami-aman · 2022-12-09T11:51:07Z

Tests/Shared/SampleData/CustomAttributesSampleData.swift

@@ -0,0 +1,18 @@
+import Foundation
+
+public enum CustomAttributesSampleData {


CustomAttributesSampleData - is this for device attributes / profile attributes or just a set of data for any scenario requiring user data ?

Set of samples for all places we accept custom attributes - identify, track, profile, device.

I'll add a comment to the file saying that. It would be helpful.

ami-aman · 2022-12-09T12:06:57Z

Tests/Shared/SampleData/SampleDataFilesUtil.swift

+// 3. Open Xcode > Window > Devices and Simulators > Select your iPhone that's plugged in > Select app in Installed App
+// section > Select "..." icon > Download container.
+// 4. You can now browse the files saved to the app. Copy and paste files from there that you care about.
+public class SampleDataFilesUtil {


I am not clear with the use of this class and it's functions. Where are the files getting saved being used ?

ami-aman · 2022-12-09T12:21:44Z

Tests/Shared/Stub/HttpRequestRunnerStub.swift

    private var responseQueue: [HttpResponse] = []
+    private var responseToAlwaysReturn: HttpResponse?


How about renaming this variable responseToAlwaysReturn, always return (what) ?

ami-aman

I have added few comments, rest all changes look good to me !

## [2.0.0-beta.1](1.2.7...2.0.0-beta.1) (2022-12-09) ### ⚠ BREAKING CHANGES * make delivered push metric more reliable * remove FCM dependency from cocoapods (#210) * singleton API only way to use SDK now (#209) ### Bug Fixes * add sdkwrapperconfig to rich push SDK config ([#226](#226)) ([e43b4cf](e43b4cf)) * do not modify custom attributes casing ([#234](#234)) ([8160fdf](8160fdf)) * fix compile time errors notification service extensions ([#214](#214)) ([bd5911b](bd5911b)) * make delivered push metric more reliable ([0478e52](0478e52)) * sdk not able to compile in ios app ([#225](#225)) ([e4d1b3f](e4d1b3f)) ### Code Refactoring * remove FCM dependency from cocoapods ([#210](#210)) ([3547076](3547076)) * singleton API only way to use SDK now ([#209](#209)) ([72b7477](72b7477))

## [2.0.0](1.2.7...2.0.0) (2022-12-13) ### ⚠ BREAKING CHANGES * make delivered push metric more reliable * remove FCM dependency from cocoapods (#210) * singleton API only way to use SDK now (#209) ### Bug Fixes * add sdkwrapperconfig to rich push SDK config ([#226](#226)) ([e43b4cf](e43b4cf)) * do not modify custom attributes casing ([#234](#234)) ([8160fdf](8160fdf)) * fix compile time errors notification service extensions ([#214](#214)) ([bd5911b](bd5911b)) * make delivered push metric more reliable ([0478e52](0478e52)) * sdk not able to compile in ios app ([#225](#225)) ([e4d1b3f](e4d1b3f)) ### Code Refactoring * remove FCM dependency from cocoapods ([#210](#210)) ([3547076](3547076)) * singleton API only way to use SDK now ([#209](#209)) ([72b7477](72b7477))

levibostian added 5 commits December 7, 2022 14:15

create failing test to reproduce

013a295

make tests for all custom attributes. fix bug

bff4c2f

make lint happy

31e8da2

simplify jsonadapter to default never convert cases

fb93516

fix tests

02b7204

add docs

eb53106

levibostian requested review from jatinn and ami-aman December 8, 2022 06:05

levibostian self-assigned this Dec 8, 2022

fix tests

ccd9167

levibostian added blocked Do not merge and removed blocked labels Dec 8, 2022

levibostian added 2 commits December 8, 2022 12:47

test backwards compatability with v1 queue.

c7dee10

refactor

667bad3

make lint happy

df7c3b7

iimprove a test

f1ab7f4

levibostian removed the Do not merge label Dec 8, 2022

levibostian added 2 commits December 8, 2022 13:35

add docs [skip ci]

4d6a148

Merge branch 'develop' into levi/fix-modify-custom-attributes

818e4f4

ami-aman reviewed Dec 9, 2022

View reviewed changes

ami-aman approved these changes Dec 9, 2022

View reviewed changes

levibostian merged commit 8160fdf into develop Dec 9, 2022

levibostian deleted the levi/fix-modify-custom-attributes branch December 9, 2022 17:05

ami-aman pushed a commit that referenced this pull request Dec 20, 2022

fix: do not modify custom attributes casing (#234)

ccfb177

ami-aman pushed a commit that referenced this pull request Dec 20, 2022

fix: do not modify custom attributes casing (#234)

c77dcd8

ami-aman pushed a commit that referenced this pull request Dec 20, 2022

fix: do not modify custom attributes casing (#234)

c197190

ami-aman pushed a commit that referenced this pull request Dec 20, 2022

fix: do not modify custom attributes casing (#234)

cb2b035

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: do not modify custom attributes casing #234

fix: do not modify custom attributes casing #234

levibostian commented Dec 8, 2022 •

edited

Loading

ami-oss-ci commented Dec 8, 2022 •

edited

Loading

Examples:

codecov bot commented Dec 8, 2022 •

edited

Loading

levibostian commented Dec 8, 2022

levibostian commented Dec 8, 2022 •

edited

Loading

ami-oss-ci commented Dec 8, 2022

ami-oss-ci commented Dec 8, 2022

levibostian commented Dec 8, 2022

ami-oss-ci commented Dec 8, 2022

ami-oss-ci commented Dec 9, 2022

ami-aman Dec 9, 2022

levibostian Dec 9, 2022

ami-aman Dec 9, 2022

levibostian Dec 9, 2022

ami-aman Dec 9, 2022

ami-aman Dec 9, 2022

ami-aman left a comment

		@@ -0,0 +1,18 @@
		import Foundation

		public enum CustomAttributesSampleData {

		private var responseQueue: [HttpResponse] = []
		private var responseToAlwaysReturn: HttpResponse?

fix: do not modify custom attributes casing #234

fix: do not modify custom attributes casing #234

Conversation

levibostian commented Dec 8, 2022 • edited Loading

Test backwards compatibility with Queue storage since Queue saves data into JSON.

ami-oss-ci commented Dec 8, 2022 • edited Loading

Examples:

codecov bot commented Dec 8, 2022 • edited Loading

Codecov Report

levibostian commented Dec 8, 2022

levibostian commented Dec 8, 2022 • edited Loading

ami-oss-ci commented Dec 8, 2022

ami-oss-ci commented Dec 8, 2022

levibostian commented Dec 8, 2022

ami-oss-ci commented Dec 8, 2022

ami-oss-ci commented Dec 9, 2022

ami-aman Dec 9, 2022

Choose a reason for hiding this comment

levibostian Dec 9, 2022

Choose a reason for hiding this comment

ami-aman Dec 9, 2022

Choose a reason for hiding this comment

levibostian Dec 9, 2022

Choose a reason for hiding this comment

ami-aman Dec 9, 2022

Choose a reason for hiding this comment

ami-aman Dec 9, 2022

Choose a reason for hiding this comment

ami-aman left a comment

Choose a reason for hiding this comment

levibostian commented Dec 8, 2022 •

edited

Loading

ami-oss-ci commented Dec 8, 2022 •

edited

Loading

codecov bot commented Dec 8, 2022 •

edited

Loading

levibostian commented Dec 8, 2022 •

edited

Loading