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

Improve display of job result and properties (followup) #2396

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0xced
Copy link
Contributor

@0xced 0xced commented Apr 19, 2024

This is a followup to #1509 which was closed on the ground that the new UseJobDetailsRenderer method could be used.

While technically possible to achieve (see https://github.com/0xced/Hangfire.TidyDashboard) it's a lot of work: reimplement the Hangfire.Dashboard.JobHistoryRenderer.SucceededRenderer method in order to prettify the JSON and the newlines and wrap a JobStorageMonitor in order to prettify the parameters.

It think this makes much more sense to have this prettifying of strings built-in to the Hangfire dashboard.

Note

I have also fixed a bug compared to the original pull request where non-string job parameters would be double html escaped.

Original description

It's quite common that job properties and/or result are strings. Since everything is serialized as JSON in the database, a string becomes quoted and has escaped characters. For displaying in the dashboard, it's nicer without the quotes and escaped characters.

Before this pull request:
HangFire-ugly

After this pull request:
HangFire-pretty

@0xced 0xced force-pushed the pretty-result-and-properties branch from 5c95a78 to 556ef2e Compare May 22, 2024 10:28
It's quite common that job properties and/or result are strings. Since everything is serialized as JSON in the database, a string becomes quoted and has escaped characters. For displaying in the dashboard, it's nicer without the quotes and escaped characters.
@0xced 0xced force-pushed the pretty-result-and-properties branch from 556ef2e to 095df4f Compare June 24, 2024 19:12
@odinserj
Copy link
Member

Hi @0xced! I'd still like to stand with the solution posted in this comment – #1509 (comment). The main reason for this is to have both parameters and return value formatted as JSON string by default to avoid confusion and present the value as is. Otherwise it might be not clear which type is behind the value, especially for numbers encoded as strings.

Currently they will be displayed as "12345", and integer-based values as 12345, proving a clear information about the actual type (string vs int). If we change that, it would be totally unclear what is the actual type behind the value.

I don't like the solution to special-case strings and forgetting other types, because it's not consistent. General solution to this problem would be possible by converting all such values from JSON to their actual type first (string/int/CustomDto), and then calling their ToString method. But not all the types have this method implemented, especially different custom DTOs, and sometimes it's impossible to identify the expected type -- for example, job parameters are converted dynamically, and no information about their actual type is stored, conversion is performed in code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants