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

Load and store embedded dataset #5370

Open
wants to merge 6 commits into
base: feature/cris/malicious-site-protection/intercept-requests
Choose a base branch
from

Conversation

CrisBarreiro
Copy link
Contributor

@CrisBarreiro CrisBarreiro commented Dec 9, 2024

Task/Issue URL: https://app.asana.com/0/1205008441501016/1208870183150417/f

Description

Load and store embedded dataset for malicious site protection

  • Note: An upcoming PR will add the remaining logic for fetching data from network, as well as the feature to update an existing dataset rather than completely replace it, as we're doing now. This PR just adds the bare minimum, so we can integrate the blocking algorithm and verify the feature works as intented

Steps to test this PR

  • Check app startup is executed normally, no ANRs are detected
  • Filter for "Failed to fetch embedded" and check no such logs exist

UI changes

no UI changes

Copy link
Contributor Author

CrisBarreiro commented Dec 9, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/load-initial-dataset branch 2 times, most recently from e99a589 to 41dbcee Compare December 10, 2024 10:26
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/intercept-requests branch from 3932394 to 13cfb3a Compare December 10, 2024 10:29
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/load-initial-dataset branch from 41dbcee to 45d1aaa Compare December 10, 2024 10:29
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/intercept-requests branch from 13cfb3a to 8d0f3b0 Compare December 10, 2024 10:49
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/load-initial-dataset branch from 45d1aaa to ca1717c Compare December 10, 2024 10:50
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/intercept-requests branch from 8d0f3b0 to 0b7f33d Compare December 10, 2024 13:37
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/load-initial-dataset branch 2 times, most recently from 212e3b7 to 7c5e5c2 Compare December 10, 2024 15:27
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/intercept-requests branch from 615f371 to 7fd8100 Compare December 11, 2024 14:03
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/load-initial-dataset branch from 7c5e5c2 to 54f233d Compare December 11, 2024 14:03
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/intercept-requests branch from 7fd8100 to c24dff3 Compare December 11, 2024 14:12
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/load-initial-dataset branch from 54f233d to 0f7697a Compare December 11, 2024 14:12
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/load-initial-dataset branch from 0f7697a to 7c8c545 Compare December 16, 2024 10:47
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/intercept-requests branch from 3fb96e8 to 601db6b Compare December 16, 2024 17:31
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/load-initial-dataset branch 2 times, most recently from d585fa1 to e310dad Compare December 17, 2024 09:21
@CrisBarreiro CrisBarreiro marked this pull request as ready for review December 17, 2024 09:42
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/intercept-requests branch from 2c3627c to cfc8cea Compare December 18, 2024 15:41
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/load-initial-dataset branch from 1ea8bd3 to 768449f Compare December 18, 2024 15:42
Copy link
Contributor

@cmonfortep cmonfortep left a comment

Choose a reason for hiding this comment

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

Left some questions about the insert, if that's the expected behavior I'm good here.

Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: you can extract some logic here to a generic method since every method has a similar structure.

private inline fun loadAndParseEmbeddedData(@rawres file: Int): T? {
return try {
val data = loadEmbeddedData(file)
val adapter = moshi.adapter(T::class.java)
adapter.fromJson(String(data, Charsets.UTF_8))
} catch (e: Exception) {
Timber.e(e, "Failed to fetch embedded data for resource: $file")
null
}
}

malwareFiltersRevision = max(lastRevision?.malwareFiltersRevision ?: 0, malwareFilterSetRevision ?: 0),
),
)
insertHashPrefixes(phishingHashPrefixes.map { HashPrefixEntity(hashPrefix = it, type = "phishing") })
Copy link
Contributor

Choose a reason for hiding this comment

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

QQ about inserts here: This will insert but not replace right? so it's additive and will replace only if there's a conflict. But if no conflict, old data will still be present. Example: if we have A, B, C, D, and receive B, D, E ...DB state will be A, B, C, D, E. Shouldn't be the new state B, D, E?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmonfortep, you're right. In a previous implementation revisions were a FK, so by removing them, we would remove filters and hash prefixes, but that's not the case anymore. I'll fix it

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/intercept-requests branch from db4d9bc to bad45a7 Compare December 19, 2024 17:10
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/malicious-site-protection/load-initial-dataset branch from 768449f to d6c864d Compare December 19, 2024 17:11
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.

2 participants