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

feature: support DbString in DapperAOT #119

Merged
merged 27 commits into from
Jul 26, 2024

Conversation

DeagleGross
Copy link
Collaborator

@DeagleGross DeagleGross commented May 1, 2024

DbString is a known type in Dapper and currently there is no dedicated support in DapperAOT for it.

Current PR does 2 things regarding DbString:

  1. in analyzer mode reports DAP48 and suggests to move from usage of DbString to DbValue attribute.
  2. properly configures DbParameter if one of parameter members is DbString by setting .Value, .Size and .DbType

Implementation notes:

  1. I have moved files in Dapper.AOT.Analyzers which are copied to the output in separate folder InGeneration
  2. I have created a single code writer for such types (PreGeneratedCodeWriter), and included the generation context options in GenerateState to have a control over which pre-generated types to include in the output. I decided to do in order to avoid writing DbStringHelpers in the output even in those cases, where DbString is not even used (basically to have more clear output)

Questions:

  1. I am pretty sure I messed up on how to get the size of DbString in ANSI encoding. How do we do it properly?
  2. What exactly do we need to do with the DbString in UpdateParameters? Do we need to call the same setup there, or just re-assigning value is fine?

Closes #89

@DeagleGross DeagleGross added enhancement New feature or request analyzer labels May 1, 2024
@DeagleGross DeagleGross requested a review from mgravell May 1, 2024 17:16
@DeagleGross DeagleGross self-assigned this May 1, 2024
@DeagleGross
Copy link
Collaborator Author

DeagleGross commented May 24, 2024

add Integration tests for dbstring (in a separate project Dapper.AOT.Test.Integration)

  • dapper.vanilla without any interception (no dapper aot)
  • dapper code with dapper aot with dbString
  • dbvalue (interception)

Copy link
Member

@mgravell mgravell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great; there's a ton of work in here, thanks; a couple of minor optional nits, but: massive thanks

docs/rules/DAP048.md Outdated Show resolved Hide resolved
src/Dapper.AOT.Analyzers/Dapper.AOT.Analyzers.csproj Outdated Show resolved Hide resolved
@DeagleGross
Copy link
Collaborator Author

Hey @mgravell,
I know you asked me to implement integration tests - I tried but I still dont have a solution yet. Filed an issue to do it in a separate PR: #122

regarding testing DbString functionality: I packed Dapper.AOT, installed in local NuGet feed and used in a console app:

  1. I can confirm that we emit a proposition to migrate from DbString to [DbValue] (only in Dapper.AOT mode)
    [module: DapperAot]
    image

  2. confirmed that query against local database works and returns the model I have in database (note: we hit the breakpoint in the generated DbStringHelpers - look at screenshot):
    image

Repro of DbString usage

@mgravell mgravell merged commit 21b383a into DapperLib:main Jul 26, 2024
1 check failed
@DeagleGross DeagleGross deleted the dmkorolev/dbstring branch July 26, 2024 09:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DbString - analyzer and runtime
2 participants