Talent-Catalog / talentcatalog

https://tctalent.org
GNU Affero General Public License v3.0
13 stars 4 forks source link

Candidates not appearing on Export file #1653

Open cazcam34 opened 5 days ago

cazcam34 commented 5 days ago

Hi <@U055YR1QT0A> <@U02NYRN57V5> <@UJ45K6M36> I am not sure why, but all my Exports from the TC are coming out blank. Only the column headings are downloading, but no data. Anyone else having this issue? This is happening on multiple saved searches.

Slack Message

cazcam34 commented 5 days ago

Debugging through this I found the code wasn't proceeding past the persistenceContextHelper.flushAndClearEntityManager(); in the exportToCSV method on the backend.

Screen Shot 2024-11-21 at 11.19.43 am.png

I looked at the history and noticed that on the 7th November we made a change to this code where we changed the entity manager from entityManager.clear();. I reverted the code back to using entityManager.clear() and found that the code now works again.

Screen Shot 2024-11-21 at 11.19.35 am.png

So I believe the issue lies in this line of code - @samschlicht I think you have a bit more experience in the area of entity manager's? Would you be able to assist?

samschlicht commented 5 days ago

@cazcam34 I definitely wouldn't put myself up as particularly knowledgeable in this area, but flush() writes to the DB any changes made to managed entities to the DB before clear() detaches them from the persistence context. Sadat made the change to prepend flush() to all instances of clear() because when we were using it in batch processing it meant that some of the changes were not being written to the DB. In this example I would imagine we can safely remove it because there aren't any changes being made.

However we should probably check other places where this is used, in case there's an issue with the PersistenceContextHelper. At first consideration I don't understand why flush() should cause an issue here.

samschlicht commented 5 days ago

Testing this and having a chance to read the error, @cazcam34, the issue with using flush() makes sense. Using Entity Manager methods means having to manage transaction boundaries ourselves (Spring takes care of it with standard repo methods).

There is no transaction open here, so flush(), which wants to keep the DB synchronised, is throwing an error.

clear() only operates in memory and so doesn't require a transaction.

The use of flush() is redundant here anyway (we're not making any changes to managed entities), so we can safely remove it. My suggestion (which I've pushed to the branch) is to add a clear-only method to the PersistenceContextHelper with clarifying doc.

Let me know what you reckon, and I guess this needs to be a hot fix but perhaps to be discussed in meeting later first?

samschlicht commented 5 days ago

As a wrapping-up exercise here, should just check the other uses of flushAndClear():

Okay, glad I checked! Found the same issue in SavedSearchService#searchCandidates, which is responsible for running stats on saved searches that use elasticsearch.

Flush is not required here (there are no changes being made to managed entities) and there's no transaction, so we're getting the same error:

Screenshot 2024-11-21 at 1 56 32 pm

I've added the same change to this method as to ExportToCsv and it's working fine now. Will add to my PR.

samschlicht commented 4 days ago

Just confirming that the PR attached here has been merged into staging with a hotfix into production -- CSV export and stats on ES working fine now. 👍