RFC/one Symbol Profile per interest name #3899
Conversation
Currently, each Interest activity creates a new Symbol Profile. This may leave the DB cluttered with multiple single-activity Symbols (see ghostfolio#3381), causing a slowness in portfolio snapshot computation and some difficulty in parsing all symbols in Admin Control > Market Data page. This is a proposal to keep a single Symbol Profile for each unique name used for an Interest-typed activity. Test Plan: 1. Create an Interest activity named "AAAA" 2. Create another Interest activity named "AAAA" 3. Verify in Market Data that only one Symbol Profile exists for both interest activities 4. Delete the first "AAAA" Interest activity 5. Verify the other activity still exists 6. Verify the "AAAA" Symbol Profile still exists 7. Delete the second "AAAA" Interest activity 8. Verify the "AAAA" Symbol Profile got deleted 9. Create an Interest activity named "BBBB" 10. Create another Interest activity named "BBBB" 11. Update one of the activities to be named "CCCC" 12. Verify one "BBBB" and one "CCCC" activities exist 12. Verify in Market Data that there are two Symbol Profiles, one for "BBBB" and one for "CCCC" 13. Rename Symbol Profile "CCCC" to "DDDD" from the Market Data page 14. Verifify the activity in Activities page shows up as "DDDD" now
|
|
||
| let symbolProfileSymbol = id; | ||
|
|
||
| if (data.type === 'INTEREST') { |
There was a problem hiding this comment.
Shouldn't it be the same behavior for all types according to #3381 (comment)?
There was a problem hiding this comment.
I wasn't sure if those comments meant "yeah, they go through the same code path when being created" or "yeah, we should reuse profiles for all those types". Given that I'm not too familiar with how non-INTEREST types are used across the codebase, I felt it was safer to start with just INTERESTs. Should we apply the same logic to all of them then?
|
|
||
| if (data.type === 'INTEREST') { | ||
| const symbolProfile = await this.prismaService.symbolProfile.findFirst({ | ||
| orderBy: { |
There was a problem hiding this comment.
Probably not necessary for findFirst
There was a problem hiding this comment.
Does findFirst implicitly picks the first in reverse chronological order? Or you mean it doesn't matter which one we pick if there are multiple with the same name? My intent with picking the last-created here was to leave old data unchanged to avoid any unforeseen issue that could possibly corrupt people's data. For instance, I have many months of "Bank Interest". After these changes, all new "Bank Interest" will be grouped withing the same Symbol Profile, but the old ones will stay as they were.
There was a problem hiding this comment.
If where is unique (see my comment on collision below), I think orderBy is not needed
| orderBy: { | ||
| createdAt: 'desc' | ||
| }, | ||
| where: { |
There was a problem hiding this comment.
There will be collisions in the cloud setup. A userId (including migration path) is needed to avoid them.
There was a problem hiding this comment.
There will be collisions in the cloud setup. A
userId(including migration path) is needed to avoid them.
You mean collisions like users A and B having the same INTEREST named "Bank Interest"? Shouldn't it be the fine, in the same way that it's fine for users A and B to have different activities pertaining the same AAPL symbol, for instance?
There was a problem hiding this comment.
Maybe it would work for INTEREST, but I see an issue for ITEM. If you and me both own a Beach House, we could not share the same asset profile because of different historical market prices.
There was a problem hiding this comment.
Ah, good point! But in that same train of thought, I could see a user having multiple ITEM items with the same name to represent different physical items, right? Should we keep ITEMs as is so as to not break that expected behavior for historical market prices?
There was a problem hiding this comment.
Yes, we could keep ITEM (and LIABILITY) as is and see them as unique.
As a result there will be one Symbol Profile per INTEREST / FEE?
There was a problem hiding this comment.
Sounds good to me, I'll update accordingly.
Just to confirm though - do you still think we should have INTEREST/FEE unique per userId or it's ok for them to be globally unique?
There was a problem hiding this comment.
I would still do it unique per userId, otherwise if I rename it would also be renamed for you.
There was a problem hiding this comment.
So, in the current implementation that would not happen. See updateOrder below. When you rename a INTEREST order, it will either connect it to an existing SymbolProfile with the new name or create a new SymbolProfile if one doesn't exist with that name.
| delete data.SymbolProfile.connect; | ||
|
|
||
| const symbolProfile = await this.prismaService.symbolProfile.findFirst({ | ||
| orderBy: { |
There was a problem hiding this comment.
Probably not necessary for findFirst
Currently, each Interest activity creates a new Symbol Profile. This may leave the DB cluttered with multiple single-activity Symbols (see #3381), causing a slowness in portfolio snapshot computation and some difficulty in parsing all symbols in Admin Control > Market Data page.
This is a proposal to keep a single Symbol Profile for each unique name used for an Interest-typed activity. What do you think?
Test Plan: