Skip to content

RUMM-1439 discard batches on invalid Client Token #535

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

Conversation

xgouchet
Copy link
Member

@xgouchet xgouchet commented Jul 6, 2021

What and why?

When an application is configured with an invalid Client Token, discard the batch to avoid retrying endlessly with the same invalid token.

@xgouchet xgouchet requested a review from a team as a code owner July 6, 2021 12:09
Copy link
Member

@mariusc83 mariusc83 left a comment

Choose a reason for hiding this comment

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

I guess you will need an unit test for this also ?

@xgouchet
Copy link
Member Author

xgouchet commented Jul 6, 2021

@mariusc83 unfortunately there's no UT already on this mechanism. I'll try and see if I can set it up but it looks that a refactor might be needed to make that class testable :/

@ncreated
Copy link
Member

ncreated commented Jul 7, 2021

Let's add tests for that as this was a reason why we let this bug enter 🙂. Although it can't be tested directly (it's a private API), we can add two behaviour tests, each observing if the data was deleted / preserved:

  • when upload finishes with acceptable code - the file is deleted,
  • when upload finishes with server error code - the file is preserved.

We can mock URLSession (with our ServerMock utility) and let it use real file storage (by using writer and reader already configured in DataUploadWorkerTests):

func testGivenDataToUpload_whenUploadFinishesWithAcceptableStatusCode_thenDataIsDeleted() {
    func test(statusCode: Int) {
        let server = ServerMock(delivery: .success(response: .mockResponseWith(statusCode: statusCode)))
        let dataUploader = DataUploader(
            urlProvider: .mockAny(),
            httpClient: HTTPClient(session: server.getInterceptedURLSession()),
            httpHeaders: .mockAny()
        )

        // Given
        writer.write(value: ["key": "value"])
        XCTAssertEqual(try temporaryDirectory.files().count, 1)

        // When
        let worker = DataUploadWorker(
            queue: uploaderQueue,
            fileReader: reader,
            dataUploader: dataUploader,
            uploadConditions: DataUploadConditions.alwaysUpload(),
            delay: DataUploadDelay(performance: UploadPerformanceMock.veryQuick),
            featureName: .mockAny()
        )
        _ = server.waitAndReturnRequests(count: 1)

        // Then
        worker.cancelSynchronously()
        XCTAssertEqual(try temporaryDirectory.files().count, 0, "When status code \(statusCode) is received, data should be deleted")
    }

    test(statusCode: (200...299).randomElement()!)
    test(statusCode: (300...399).randomElement()!)
    test(statusCode: (400...499).randomElement()!)
    test(statusCode: 403)
}

func testGivenDataToUpload_whenUploadFinishesWithServerErrorStatusCode_thenDataIsPreserved() {
    let statusCode = (500...599).randomElement()!
    let server = ServerMock(delivery: .success(response: .mockResponseWith(statusCode: statusCode)))
    let dataUploader = DataUploader(
        urlProvider: .mockAny(),
        httpClient: HTTPClient(session: server.getInterceptedURLSession()),
        httpHeaders: .mockAny()
    )

    // Given
    writer.write(value: ["key": "value"])
    XCTAssertEqual(try temporaryDirectory.files().count, 1)

    // When
    let worker = DataUploadWorker(
        queue: uploaderQueue,
        fileReader: reader,
        dataUploader: dataUploader,
        uploadConditions: DataUploadConditions.alwaysUpload(),
        delay: DataUploadDelay(performance: UploadPerformanceMock.veryQuick),
        featureName: .mockAny()
    )
    _ = server.waitAndReturnRequests(count: 1)

    // Then
    worker.cancelSynchronously()
    XCTAssertEqual(try temporaryDirectory.files().count, 1, "When status code \(statusCode) is received, data should be preserved")
}

Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Needs tests #535 (comment)

@xgouchet xgouchet requested review from ncreated July 7, 2021 08:56
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

LG 🚀

@xgouchet xgouchet merged commit bd67d45 into master Jul 9, 2021
@xgouchet xgouchet deleted the xgouchet/RUMM-1439/discard_batches_on_invalid_client_token branch July 9, 2021 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants