feat(get-domain-from-backend) Use new Helium API to fetch backend domain #WPB-11287 #27

spoonman01 · 2024-09-30T10:25:08Z

Update Helium to 1.4.1
Add assertions to major tests
Remove Consts.java from tests

PR Submission Checklist for internal contributors

The PR Title
- conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
- contains a reference JIRA issue number like SQPIT-764
- answers the question: If merged, this PR will: ... ³
The PR Description
- is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

Using old version of Helium

Solutions

Update Helium in order to leverage new API and objects

Dependencies (Optional)

Smaller PRs on Helium and Xenon

PR Post Submission Checklist for internal contributors (Optional)

Wire's Github Workflow has automatically linked the PR to a JIRA issue

References

https://sparkbox.com/foundry/semantic_commit_messages
https://github.com/wireapp/.github#usage
E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

* Update Helium to 1.4.1 * Add assertions to major tests * Remove Consts.java from tests

* Update Helium to 1.4.1 * Add assertions to major tests * Remove Consts.java from tests * Add call to get api version and default domain

marcoconti83 · 2024-10-01T14:51:59Z

src/main/java/com/wire/bots/hold/MessageHandler.java

-            final byte[] assetData = client.downloadAsset(msg.getAssetId(), msg.getAssetToken(), msg.getSha256(), msg.getOtrKey());
+            final byte[] assetData = client.downloadAsset(
+                msg.getAssetId(),
+                null, // TODO(WPB-11287): Change null to default domain


It shouldn't be the default domain, it should be the domain of the message you got (well, the domain fo the sender). Someome might have send an image from another domain.

Yes, the comment might be a bit misleading to what it needs to be done, but at least we have an overview of where it will need to be changed as well when we do the database migrations from UUID to QualifiedID (id, domain)

marcoconti83 · 2024-10-01T14:53:11Z

src/main/java/com/wire/bots/hold/NotificationProcessor.java

        try {
+            // todo get exception


What does "todo get exception" means?
TODO: handle exceptions on the GET /api-version request?

Indeed we forgot, but it is actually the handling of HttpException for retrieving notifications as now we are using the API call from Helium and not doing the implementation ourselves in LegalHold.

Added the correct catch

marcoconti83 · 2024-10-01T14:55:00Z

src/main/java/com/wire/bots/hold/NotificationProcessor.java

-            NotificationList notificationList = retrieveNotifications(device);
+            NotificationList notificationList = api.retrieveNotifications(
+                device.clientId,
+                device.last,


This is interesting, is device.last the last known notification for the device (client)?
IF so, what was this doing before, downloading ALL notifications every time?

It is the last known notification for the device.

Before it was still using the last known notification, but the handling was being done in a private implementation in LegalHold (so we only passed the device object info), now we call retrieveNotifications from Helium, so explicitly passing all the necessary params e not only the object.

You can check the private implementation from lines 146 to 170 that were deleted in this file in order to use the api call in Helium

marcoconti83 · 2024-10-01T15:01:33Z

src/main/java/com/wire/bots/hold/utils/Cache.java

-    private static final ConcurrentHashMap<UUID, File> assets = new ConcurrentHashMap<>();//<messageId, File>
-    private static final ConcurrentHashMap<UUID, User> users = new ConcurrentHashMap<>();//<userId, User>
-    private static final ConcurrentHashMap<UUID, File> profiles = new ConcurrentHashMap<>();//<userId, Picture>
+    private static String DEFAULT_DOMAIN = null;


I don't have enough context, but it seems to me that this is an in-memory cache? Or is it persisted?

If it's in-memory only, perhaps the default domain value should be persisted instead, and perhaps the app should throw a fatal error if it has previously stored data (users, conversations, ...) with one domain, but at start up checks the domain with the backend and it's now aa different domain.

Otherwise the code might make a lot of wrong assumptions when looking at old local data. If the data (users, conversations, ...) was stored with null domain when the default domain was A, but now the defalt domain is B, the data will be inconsistent.

Or do we always store the domain explicitly in the data that is persisted?

Yes, it is only stored in-memory.

You are correct in the case of the application being shutdown and then changing the domain, when starting again it will assume a different domain and will have inconsistent data (domain) between values that was previously stored.

If that is an issue, then we can persist the default domain in the database and throw a fatal error if when restarting the service and the fetched domain from /api-version is different from the one persisted, then we can throw a fatal error.

Should we have a quick meeting between me, you and @spoonman01 about the final approach? or can we continue with what was said:

If in DB default domain is null, fetch from /api-version and persist it.

if in DB default domain is NOT null, when fetching from /api-version and the values are different, we throw a fatal exception.

To be honest, this situation (the domain changes) is extremely unlikely, but I like to do a bit of defensive programming. So we should look at this with a risk-cost analysis. Is the change to consider this scenrio a substantial change that will take a while to implement? Then perhaps we accept the risk. Is it relatively easy to implement? Then I would implement it.

We created a new table called Metadata that holds 2 fields :key and value (so it can also be used later if we need more metadata/configs to be saved)

And also created a file called FallbackDomainFetcher where it runs on service start to verify if it contains a value in Cache, otherwise fetch from the API and compare against what is saved in the database (if there is any domain saved).

In case the domain received from the API is different from the one in the database, then we are throwing a RuntimeException.

* Update Helium to 1.4.1 * Add assertions to major tests * Remove Consts.java from tests * Add call to get api version and default domain * Persist fallback domain to database * Add FallbackDomainFetcher to handle fetch from cache, database and API

src/main/resources/db/migration/V107__add_metadata_table.sql

* Update Helium to 1.4.1 * Add assertions to major tests * Remove Consts.java from tests * Add call to get api version and default domain * Persist fallback domain to database * Add FallbackDomainFetcher to handle fetch from cache, database and API * Properly receive thrown exception from dropwizard task execution * Add Mockito for testing mocks * Add JavaDoc for FallbackDomainFetcher

# Conflicts: # src/main/java/com/wire/bots/hold/Service.java

sonarcloud · 2024-10-07T13:01:45Z

Quality Gate passed

Issues
35 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

marcoconti83

Changed the status to approved to remove the blocker, but @spoonman01 will review the code

spoonman01 · 2024-10-07T15:31:01Z

Can't approve formally because I was the one opening the PR, because of some Git issues last week.
FTR: Approving all changes here

feat(get-domain-from-backend) Use new Helium API to fetch backend domain

7f59827

* Update Helium to 1.4.1 * Add assertions to major tests * Remove Consts.java from tests

spoonman01 assigned alexandreferris Sep 30, 2024

echoes-hq bot added the echoes/initiative: federation-wire-cloud Activate Federation with MLS on Wire Cloud label Sep 30, 2024

feat(get-domain-from-backend) Use new Helium API to fetch backend domain

a051559

* Update Helium to 1.4.1 * Add assertions to major tests * Remove Consts.java from tests * Add call to get api version and default domain

marcoconti83 requested changes Oct 1, 2024

View reviewed changes

spoonman01 commented Oct 7, 2024

View reviewed changes

src/main/resources/db/migration/V107__add_metadata_table.sql Outdated Show resolved Hide resolved

alexandreferris added 3 commits October 7, 2024 14:59

Merge remote-tracking branch 'origin/WPB-11287' into WPB-11287

dd5ac09

# Conflicts: # src/main/java/com/wire/bots/hold/Service.java

alexandreferris requested a review from marcoconti83 October 7, 2024 15:02

alexandreferris mentioned this pull request Oct 7, 2024

feat(add-api-v1) Add new resources handling qualified id requests from the backend #WPB-11299 #28

Merged

6 tasks

marcoconti83 approved these changes Oct 7, 2024

View reviewed changes

alexandreferris merged commit 00b299e into staging Oct 7, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(get-domain-from-backend) Use new Helium API to fetch backend domain #WPB-11287 #27

feat(get-domain-from-backend) Use new Helium API to fetch backend domain #WPB-11287 #27

spoonman01 commented Sep 30, 2024

marcoconti83 Oct 1, 2024

alexandreferris Oct 2, 2024

marcoconti83 Oct 1, 2024

alexandreferris Oct 2, 2024

marcoconti83 Oct 1, 2024

alexandreferris Oct 2, 2024

marcoconti83 Oct 1, 2024

alexandreferris Oct 2, 2024

marcoconti83 Oct 2, 2024

alexandreferris Oct 7, 2024

sonarcloud bot commented Oct 7, 2024

marcoconti83 left a comment

spoonman01 commented Oct 7, 2024

feat(get-domain-from-backend) Use new Helium API to fetch backend domain #WPB-11287 #27

feat(get-domain-from-backend) Use new Helium API to fetch backend domain #WPB-11287 #27

Conversation

spoonman01 commented Sep 30, 2024

PR Submission Checklist for internal contributors

What's new in this PR?

Issues

Solutions

Dependencies (Optional)

PR Post Submission Checklist for internal contributors (Optional)

References

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 7, 2024

Quality Gate passed

marcoconti83 left a comment

Choose a reason for hiding this comment

spoonman01 commented Oct 7, 2024