[Hack Week] ApiFaker: introduce new module and logic #13052

hichamboushaba · 2024-12-03T11:35:36Z

Description

As part of my Hack Week, I'm bringing back the ApiFaker project paqN3M-SI-p2, and I'm splitting the feature into multiple PRs.

This PR adds the apifaker gradle module, and with it the following changes:

A Room database to hold the list of faked endpoints.
The EndpointProcessor class is responsible for matching and finding the faked endpoints.
The OkHttp interceptor that replaces the response when needed.

Steps to reproduce

Code review and green CI should be enough.

But if you prefer to do some tests, you can use the following PR #13070

I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

dangermattic · 2024-12-03T11:36:29Z

	1 Error
🚫	This PR is tagged with `status: do not merge` label(s).

	4 Warnings
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	Class `ApiFakerInterceptor` is missing tests, but `unit-tests-exemption` label was set to ignore this.
⚠️	Class `ApiFakerDatabase` is missing tests, but `unit-tests-exemption` label was set to ignore this.
⚠️	Class `EndpointTypeConverter` is missing tests, but `unit-tests-exemption` label was set to ignore this.

Generated by 🚫 Danger

wpmobilebot · 2024-12-03T11:40:42Z

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`3d1c8c9`
	Direct Download	`woocommerce-wear-prototype-build-pr13052-3d1c8c9.apk`

wpmobilebot · 2024-12-03T11:47:33Z

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`3d1c8c9`
	Direct Download	`woocommerce-prototype-build-pr13052-3d1c8c9.apk`

codecov-commenter · 2024-12-03T14:40:08Z

Codecov Report

Attention: Patch coverage is 0% with 126 lines in your changes missing coverage. Please review.

Project coverage is 40.26%. Comparing base (c944700) to head (3d1c8c9).
Report is 2 commits behind head on task/api-faker-prep.

Files with missing lines	Patch %	Lines
.../woocommerce/android/apifaker/EndpointProcessor.kt	0.00%	63 Missing ⚠️
...oocommerce/android/apifaker/ApiFakerInterceptor.kt	0.00%	19 Missing ⚠️
...merce/android/apifaker/db/EndpointTypeConverter.kt	0.00%	10 Missing ⚠️
...oocommerce/android/apifaker/db/ApiFakerDatabase.kt	0.00%	8 Missing ⚠️
...com/woocommerce/android/apifaker/models/Request.kt	0.00%	7 Missing ⚠️
...com/woocommerce/android/apifaker/db/EndpointDao.kt	0.00%	5 Missing ⚠️
...om/woocommerce/android/apifaker/models/Response.kt	0.00%	5 Missing ⚠️
...commerce/android/apifaker/models/MockedEndpoint.kt	0.00%	3 Missing ⚠️
...com/woocommerce/android/apifaker/models/ApiType.kt	0.00%	2 Missing ⚠️
.../woocommerce/android/apifaker/models/HttpMethod.kt	0.00%	2 Missing ⚠️
... and 1 more

Additional details and impacted files

@@                    Coverage Diff                    @@
##             task/api-faker-prep   #13052      +/-   ##
=========================================================
- Coverage                  40.33%   40.26%   -0.07%     
  Complexity                  6154     6154              
=========================================================
  Files                       1287     1298      +11     
  Lines                      74121    74247     +126     
  Branches                   10140    10157      +17     
=========================================================
  Hits                       29895    29895              
- Misses                     41634    41760     +126     
  Partials                    2592     2592

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This commit adds logic for extracting the method from the path, and also the logic to wrap the body using `data` property when needed

hichamboushaba · 2024-12-05T09:21:48Z

libs/apifaker/src/main/java/com/woocommerce/android/apifaker/EndpointProcessor.kt

+    private val jsonObjectProvider: JSONObjectProvider
+) {
+    fun fakeRequestIfNeeded(request: Request): Response? {
+        // TODO match against method and query parameters too


This TODO here is not accurate, as part of this PR we are already matching against the HTTP method, the comment is fixed in next PRs, to avoid issues with merge conflicts, let's ignore it here.

…requests

hichamboushaba · 2024-12-05T09:28:33Z

libs/apifaker/src/main/java/com/woocommerce/android/apifaker/EndpointProcessor.kt

+            val (path, method, body) = if (method == "GET") {
+                Triple(
+                    url.queryParameter("path")!!.substringBefore("&"),
+                    url.queryParameter("_method") ?: "GET",


For Jetpack Tunneled GET requests, the HTTP method is sent as a query parameter _method, see https://github.com/wordpress-mobile/WordPress-FluxC-Android/blob/64479b5648444c945dccae776f596daf192f4b87/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpcom/jetpacktunnel/JetpackTunnelGsonRequest.kt#L249

This will generally be always GET, but to avoid hardcoding it, I'm reading it from the parameter when it exists.

hichamboushaba · 2024-12-05T09:29:21Z

libs/apifaker/src/main/java/com/woocommerce/android/apifaker/EndpointProcessor.kt

+                val jsonObject = jsonObjectProvider.parseString(originalBody)
+                val pathParts = jsonObject.getString("path").split("&")
+                val path = pathParts[0]
+                val method = pathParts.firstOrNull { it.startsWith("_method") }?.split("=")?.getOrNull(1) ?: "POST"


For Jetpack Tunneled POST requests, the actual endpoint HTTP method will be past with the json property path, see: https://github.com/wordpress-mobile/WordPress-FluxC-Android/blob/64479b5648444c945dccae776f596daf192f4b87/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpcom/jetpacktunnel/JetpackTunnelGsonRequest.kt#L265

hichamboushaba · 2024-12-05T09:30:58Z

libs/apifaker/src/main/java/com/woocommerce/android/apifaker/EndpointProcessor.kt

+            url.encodedPath.trimEnd('/').matches(Regex(JETPACK_TUNNEL_REGEX)) &&
+            !startsWith("{\"data\":")
+        ) {
+            "{\"data\": $this}"


Jetpack Tunnel sends responses wrapped in a data property, this logic makes it easier for faking those without the wrap, which should work for both site credentials login and Jetpack login.

This brings up the same question from above. What if we don't want to have JSON formatted body with data { } as the root of if? Like the example mentioned above when simulating an error that returns html?

As said in the other thread, I would love to keep this as is right now, changing it would require more testing.

Depending on the developer's needs, we can revisit this in the future.

hichamboushaba · 2024-12-05T09:32:45Z

.buildkite/commands/run-unit-tests.sh

@@ -2,7 +2,7 @@

 echo "--- 🧪 Testing"
 set +e
-./gradlew testJalapenoDebugUnitTest lib:cardreader:testDebugUnitTest lib:iap:testDebugUnitTest
+./gradlew testJalapenoDebugUnitTest testDebugUnitTest


This allows running unit tests for all modules instead of having to list all of them

Not a developer

JorgeMucientes

Great job @hichamboushaba, looking forward to see this in action in subsequent PRs. Code looks clean and well organised. I just left a couple minor questions, but nothing blocking.

JorgeMucientes · 2024-12-09T15:47:58Z

libs/apifaker/build.gradle

+            includeGroup "org.wordpress"
+            includeGroup "org.wordpress.fluxc"
+            includeGroup "org.wordpress.fluxc.plugins"
+            includeGroup "org.wordpress.wellsql"
+            includeGroup "org.wordpress.mediapicker"
+            includeGroup "com.automattic"
+            includeGroup "com.automattic.tracks"


Do you need all these? I see you only add FluxC as dependency (which also makes sense since it's where all the requests happen). But why include tracks, mediapicker, wellsql, wordpress ?

I don't like this either, but I tried to just keep the same setup as what we have in the other modules, personally I think this code should be in the root build.gradle file, or better in a convention plugin, but since this is not the setup we have yet, I prefer to keep the same setup as the other modules here, as the includeGroup statements won't have any impact here, they are just used for defining the repo itself.

JorgeMucientes · 2024-12-09T15:58:30Z

libs/apifaker/src/main/java/com/woocommerce/android/apifaker/ApiFakerInterceptor.kt

+                .protocol(Protocol.HTTP_1_1)
+                .message("Fake Response")
+                .code(fakeResponse.statusCode)
+                // TODO check if it's safe to always use JSON as the content type


Since the body of the faked request is anything the user inputs from the app, wouldn't it make more sense to leave MediaType null? For example sometimes we get and html body for API errors. Haven't tested yet, but what happens if we keep this application/json content type and enter non json body?

This is a good point; but for now I'll keep it like this, my rationale for hardcording application/json here is that all of the API requests we have in the app use JSON responses, the HTML errors are an exception that can be caused by a specific site setup that returns an error before the site handles the error.

what happens if we keep this application/json content type and enter non json body?

It should work as you expect from the app, the upper layer will try to deserialize the content, and would fail, then return a JSON error, this is what happens when we receive HTML instead of JSON (for example this case: p1723565504566789/1720009283.216679-slack-C6H8C3G23 API org.json.JSONException: Value <!DOCTYPE of type java.lang.String cannot be converted to JSONObject)

JorgeMucientes · 2024-12-09T16:16:20Z

libs/apifaker/src/main/java/com/woocommerce/android/apifaker/EndpointProcessor.kt

+            url.encodedPath.trimEnd('/').matches(Regex(JETPACK_TUNNEL_REGEX)) &&
+            !startsWith("{\"data\":")
+        ) {
+            "{\"data\": $this}"


This brings up the same question from above. What if we don't want to have JSON formatted body with data { } as the root of if? Like the example mentioned above when simulating an error that returns html?

hichamboushaba added type: task An internally driven task. milestone-not-required Prevents the Milestone check to fail the build labels Dec 3, 2024

hichamboushaba force-pushed the task/api-faker-prep branch from 010ba46 to 5dc003f Compare December 3, 2024 11:44

hichamboushaba force-pushed the task/api-faker-prep branch from 8ea9816 to c944700 Compare December 3, 2024 13:20

hichamboushaba force-pushed the task/api-faker-1 branch from 07950ea to 3f4341b Compare December 3, 2024 14:10

hichamboushaba added the unit-tests-exemption label Dec 3, 2024

hichamboushaba added 10 commits December 3, 2024 15:46

Create a new module for the Api Faker

d64107d

Add Room DB for holding the endpoints

61c9e55

Add initial logic for intercepting and faking responses

571b1b4

Some changes to the DB schema

bea598f

Handle extracting data from the request for matching

e3c4cbb

Add some logging, and handle errors in parsing

dccd118

Add some improvements to the DB schema and dao

cbf04b6

Rename classes

a9ad0ca

Fix detekt issues

9864ff6

Run unit tests of all libraries in CI

7586064

hichamboushaba force-pushed the task/api-faker-1 branch from 3f4341b to 7586064 Compare December 3, 2024 14:46

hichamboushaba added 4 commits December 3, 2024 16:01

Disable schema export

97f8325

Enable destructive migration

566bb53

Improve the query statement

cd04882

Add ability to delete requests

e637209

hichamboushaba force-pushed the task/api-faker-1 branch from dc8591c to 350bcf2 Compare December 4, 2024 10:52

Add HttpMethod to the DB entities

1eb669e

hichamboushaba force-pushed the task/api-faker-1 branch 2 times, most recently from 185c0cb to c5117e3 Compare December 4, 2024 13:43

Fix endpoint extraction from Jetpack tunnel endpoints

9a5c208

hichamboushaba force-pushed the task/api-faker-1 branch from c5117e3 to 9a5c208 Compare December 4, 2024 13:55

hichamboushaba added 2 commits December 4, 2024 15:01

Fix detekt issue

124ecf7

Improve logic of handling Jetpack tunnel endpoints

4982268

This commit adds logic for extracting the method from the path, and also the logic to wrap the body using `data` property when needed

hichamboushaba changed the title ~~[Hack Week] Api Faker: introduce new module and logic~~ [Hack Week] ApiFaker: introduce new module and logic Dec 4, 2024

hichamboushaba removed the milestone-not-required Prevents the Milestone check to fail the build label Dec 5, 2024

hichamboushaba added this to the 21.3 milestone Dec 5, 2024

hichamboushaba commented Dec 5, 2024

View reviewed changes

Improve the logic of extracting the http method for Jetpack tunneled …

73ba162

…requests

hichamboushaba commented Dec 5, 2024

View reviewed changes

Mark ApiFaker log tag as internal

3d1c8c9

hichamboushaba commented Dec 5, 2024

View reviewed changes

hichamboushaba added the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Dec 5, 2024

This comment was marked as spam.

Sign in to view

hichamboushaba requested review from hafizrahman, irfano and JorgeMucientes December 5, 2024 11:16

hichamboushaba marked this pull request as ready for review December 5, 2024 11:16

JorgeMucientes self-assigned this Dec 6, 2024

Base automatically changed from task/api-faker-prep to trunk December 6, 2024 16:47

JorgeMucientes approved these changes Dec 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Hack Week] ApiFaker: introduce new module and logic #13052

[Hack Week] ApiFaker: introduce new module and logic #13052

hichamboushaba commented Dec 3, 2024 •

edited by JorgeMucientes

Loading

dangermattic commented Dec 3, 2024 •

edited

Loading

wpmobilebot commented Dec 3, 2024 •

edited

Loading

wpmobilebot commented Dec 3, 2024 •

edited

Loading

codecov-commenter commented Dec 3, 2024 •

edited

Loading

hichamboushaba Dec 5, 2024

hichamboushaba Dec 5, 2024

hichamboushaba Dec 5, 2024

hichamboushaba Dec 5, 2024

JorgeMucientes Dec 9, 2024

hichamboushaba Dec 10, 2024

hichamboushaba Dec 5, 2024

This comment was marked as spam.

This comment was marked as spam.

JorgeMucientes left a comment

JorgeMucientes Dec 9, 2024

hichamboushaba Dec 10, 2024

JorgeMucientes Dec 9, 2024

hichamboushaba Dec 10, 2024

JorgeMucientes Dec 9, 2024

[Hack Week] ApiFaker: introduce new module and logic #13052

Are you sure you want to change the base?

[Hack Week] ApiFaker: introduce new module and logic #13052

Conversation

hichamboushaba commented Dec 3, 2024 • edited by JorgeMucientes Loading

Description

Steps to reproduce

Reviewer (or Author, in the case of optional code reviews):

dangermattic commented Dec 3, 2024 • edited Loading

wpmobilebot commented Dec 3, 2024 • edited Loading

wpmobilebot commented Dec 3, 2024 • edited Loading

codecov-commenter commented Dec 3, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

JorgeMucientes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hichamboushaba commented Dec 3, 2024 •

edited by JorgeMucientes

Loading

dangermattic commented Dec 3, 2024 •

edited

Loading

wpmobilebot commented Dec 3, 2024 •

edited

Loading

wpmobilebot commented Dec 3, 2024 •

edited

Loading

codecov-commenter commented Dec 3, 2024 •

edited

Loading