Skip to content

Add index to message on receiver_id, read_ #20

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jon-m-craig
Copy link
Contributor

As discussed on the forum, the countUnreadMessagesForCurrentUser() method polls the DB by receiver_id and read_ in order to get said count. But there is currently no index to make that query perform well, so... adding it would be a good idea.

@mariodavid
Copy link
Owner

Hi,

thanks for the PR. Unfortunately, as is I cannot merge it. In fact, CUBA does not use this JPA mechanism to define indexes.

Instead, it uses the DB script update mechanism to generate SQL files for the different DBs. In Studio you find the section "Indexes" for an entity:
Bildschirmfoto 2020-04-10 um 18 32 12

The following things are missing:

  • the index has to be created via SQL file generation
  • the index also needs to include the delete_ts column (not 100% sure about it) to treat the deleted items different than the non-deleted. But actually I'm not quite sure about that
  • the SQL files have to be generated for all the supported DBMS systems that the addon supports
  • the SQL files have to be generated for the create case as well as the update case

If you want to dig into that a little deeper, feel free to do so. I will guide you wherever I can. In order to spin up the different DBMS systems, you can look at this markdown file, which explains on how to start the different docker containers: https://github.com/mariodavid/cuba-component-user-inbox/blob/master/create-databases.md

Cheers
Mario

@jon-m-craig
Copy link
Contributor Author

Aha that makes total sense. I didn't know how that would work within the context of an add-on. I'll think about doing that. The index will pretty much be required in my usage anyway.

@mariodavid
Copy link
Owner

I just tried on my own for the petclinic. And in fact you are partly right. Studio changes the table annotation (probably as you did). But when you go to Menu > CUBA > Generate DB Scrips - it will also additionally generate those (for the currently setup DB type). So you were probably already on the right track.

@jon-m-craig
Copy link
Contributor Author

Yeah... when you're working on a normal project, you can go the indexes tab of the entity and make the indexes, then CUBA --> Gen scripts and then update, and it all works. I didn't know how it would work in the context of an add-on though, since I've never made or worked on one.

@mariodavid
Copy link
Owner

In fact, from this perspective, an addon is exactly the same as an application. In fact, every application can be transformed into an addon and it will just generate a XML descriptor. Even CUBA core itself is nothing more than an addon.

It is a little cumbersome to generate the scripts for all DBs and test the changes. Therefore I was not super keen to just go ahead and do it.

After thinking about the delete_ts column: I think you have to add it. Because what we would like to achieve is that this query SELECT count(*) FROM DDCUI_MESSAGE WHERE read_ = true AND receiver_id = '26ce275a-b7ec-416c-ba44-32b27179e890' AND delete_ts = null. The AND delete_ts = null is automatically added by the dataManager, because it filters for only non-soft-deleted instances.

In order to make the query fast, we have to add all columns to the index.

@jon-m-craig
Copy link
Contributor Author

Interestingly (and oddly), the indexes automatically added by CUBA never include delete_ts either. I wonder if that's an oversight by the CUBA team or if they didn't figure it was needed since they assume people will periodically purge the soft-deleted records, making it not as much of an issue?

@jon-m-craig
Copy link
Contributor Author

I've been playing with a better way to handle the counter/etc entirely. Just for testing purposes I extended MessageServiceBean and overrode the main sendMessageFromuser method (the 5-argument one that does all the work) and using the Global Events add-on, published an event that contains the sender, receiver, and so on - and in the ExtMainWindow I listen for said event and if the user logged in there matches the receiver of the event from the message service... well, there you go - increment the counter and do whatever else one might want. (Including in our specific case, we want to throw a notification up.)

This gets rid of the need for an index and also doesn't hit the DB.

What do you think?

@mariodavid
Copy link
Owner

Hi,

but if you only rely on the messages send around, how do you ensure that the users, that are not logged in at the time the message is received still get the information about their unread messages?

I think this transient behavior is not really what is currently implemented via the menu badge.

Or do I miss something?

Cheers
Mario

@jon-m-craig
Copy link
Contributor Author

The first call to the original get of the unread messages is still in the init event, so a user logging in gets their count pulled that way.

So yeah the index will still be needed. What do you make of the fact that none of the CUBA-generated indexes contain delete_ts? You seem to be pretty close with the team.

Maybe they just assume people keep the soft-deleted records pruned?

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