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

[4.x] Attempt to fix model hydration #1174

Merged
merged 1 commit into from
Feb 1, 2022
Merged

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Jan 31, 2022

This is an attempt to fix #1081 & #1068.

#1000 introduced the ability to record how many hydrations of a specific model occurred. This however, does not seem to function properly on Vapor specifically. It was originally also post-reported on the related PR but given the nature of it being Vapor specific it was very hard to reproduce for the OP.

My analysis leads me to believe that the saving of the incoming entry occurs and then afterwards, for some reason unknown, Telescope still attempts to hydrate a model when being run on Vapor. Because the incoming entry's content has already been json encoded here, it's now a string and the following exception occurs for people:

Cannot access offset of type string on string

I think this is because of some processing that happens after recording the entry which hydrates the same model again. It seems Vapor specific but most likely it's specifically due to any way an app handles processing after Telescope recording. However I could not in any way reproduce this locally.

Therefor I'm not 100% sure this fix will provide a solution to the aforementioned issues. Someone managed to solve it by modifying the DatabaseEntriesRepository but I don't think that's the correct place since this is specific to the ModelWatcher. I do not entirely know if Telescope will attempt to save the new entries again or if it'll ignore it.

I'd appreciate it if anyone from the related issues could attempt to deploy a fork of Telescope with this to see if that solves the issue for them on Vapor.

@driesvints driesvints changed the title Attempt to fix model hydration [4.x] Attempt to fix model hydration Jan 31, 2022
@daison12006013
Copy link

daison12006013 commented Jan 31, 2022

Hi from #1068,

Based on your changes, it is similar to what I did.

My work around was to extend the class and modify the config to use this class and I am checking first if the content was string then just json_decode it to be an array.

Perhaps that should work, like I said before this is somewhat a peculiar case that doesn't occur all the time.

Since the day I posted that ... the solution I've had made, up until now I never encounter such issue.

  • Looks good to me

@taylorotwell
Copy link
Member

taylorotwell commented Jan 31, 2022

The only way I see that this could happen is if:

A) startRecording is somehow called during the withoutRecording callback in Telescope@store and recording is re-enabled when it shouldn't be.

B) The after storage hooks are not being invoked because of some exception being thrown within Telescope@store before the after hooks can be invoked at the end of the callback.

The ModelWatcher class registers an "after storage" hook that flushes all of its hydrationEntries. The only place those are serialized to JSON strings is during the database storage process. So, somehow a model hydration event is being recorded within the withoutRecording callback, after storage has happened, but before the after storage callbacks have been invoked.

It's hard for me to imagine how that could be happening but I guess it is somehow? A and B noted above are the only situations I can think of that could cause this but we would need to probably place some logging statements on an application running on Vapor to try to diagnose which one is happening.

@driesvints
Copy link
Member Author

@taylorotwell right. In that case we'd need to ask one of the involved people to adjust Telescope to place the logging.

@taylorotwell taylorotwell merged commit e6c79e9 into 4.x Feb 1, 2022
@taylorotwell
Copy link
Member

Will merge this for now but would still like to discover the root cause of why this is happening if someone is willing to put some debug statements in place.

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.

Telescope on vapor issue
3 participants