-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add Server Name to Logs and Enables the Searching of Them + Changelog #53
base: master
Are you sure you want to change the base?
Add Server Name to Logs and Enables the Searching of Them + Changelog #53
Conversation
|
||
public class DapperDBContext : DbContext | ||
{ | ||
public DapperDBContext(DbContextOptions options) : base(options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you defining a new database context like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik that's because dapper can't use the same context to get the db connection as efcore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had no issues doing this myself when I've tried.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dapper tries to close the connection using the efcore context and that causes problems, ill give it another go to see if I can fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't make sense, Dapper doesn't control connection lifetimes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still pretty new to this whole thing so I'm gonna keep working on it
{ | ||
await connection.OpenAsync(); | ||
|
||
var result = await connection.QueryAsync<AdminLog, string, WebAdminLog>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JsonDocument
instances for the logs need to be manually disposed so they can be pooled. I believed EF Core did it for us before, but now that's not an option you'll have to take care of it yourself somewhere.
I made this PR less shit, fixed everything that was asked couldnt figure out how to use dapper without using a seperate context, please halp |
public LogImpact Impact { get; set; } | ||
public DateTime Date { get; set; } | ||
public string Message { get; set; } = default!; | ||
public JsonDocument Json { get; set; } = default!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're still not disposing of the JsonDocument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i forgor
Server Searching and Server name now show up, this was way harder than originally thought to get working.
Plus we got a changelog on the settings page, and cleaned the settings page up a bit (updated cuase the first one sucked)
Also added Dapper a pretty sweet micro ORM
closes #41 and #9
Also should have made the logs page overall faster, it used to try and search the whole players catalog to find a null player every time the logs page was loaded, now it only does it if you search for a player