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

Update mapping macro to support case-insensitive comparisons when matching column name to property name #195

Closed

Conversation

lachlan
Copy link
Contributor

@lachlan lachlan commented Nov 8, 2023

This pull request adds a case_sensitive : Bool parameter to the mapping macro, which defaults to true to maintain current behaviour, however when set to false it will change the column name to property name matching to be case-insensitive such that any differences in column name case used in the SQL or stored procedure that produced the result set will be ignored and still match the relevant property.

DB.mapping({ foo: String }) # => will match column name `foo` only
DB.mapping({ foo: String }, case_sensitive: true) # => will match column name `foo` only
DB.mapping({ foo: String }, case_sensitive: false) # => will match column name `foo` or `FOO` or `Foo` or `fOo` etc.

comparisons when matching column name to property name

Column to property name matching remains case-sensitive by default,
but the matching can be set to be case-insensitive as follows:

    # will match column names "foo" or "Foo" or "fOo" or FOO" etc.
    # to property name "foo"
    DB.mapping({ foo: String }, case_sensitive: false)
@bcardiff
Copy link
Member

bcardiff commented Nov 8, 2023

I see how some conventions in the database might not match the ones in crystal. Casing is one. The usage of underscores is another. A column named ClientId might be wanted to be mapped to a client_id property.

Wouldn't be better to offer a more general transform hook for this?

@straight-shoota
Copy link
Member

Isn't that just the key property?

However I'm a bit surprised that DB.mapping is still a thing. DB::Serializable has been around for a long time. The equivalents JSON.mapping and YAML.mapping in stdlib are long gone and replaced by *::Serializable.

@bcardiff
Copy link
Member

bcardiff commented Nov 8, 2023

Good point. This is covered by serializable. We can move mapping out to community and let it evolve there IMO.

@HertzDevil
Copy link

I believe the JSON::Serializable equivalent for this feature request is crystal-lang/crystal#10793

@straight-shoota
Copy link
Member

@bcardiff Actually DB.mapping has a key property as well (ref https://crystal-lang.github.io/crystal-db/api/0.12.0/DB.html#mapping%28properties%2Cstrict%3Dtrue%29-macro).

DB.mapping({ foo: {type: String, key: "FooID" } })

@lachlan
Copy link
Contributor Author

lachlan commented Nov 8, 2023

My use case is a bit esoteric, I wanted to use result sets for the same "objects" that are returned by a couple of different SQL Server stored procedures that I don't control and cannot change (shipped with a commercial product), and they use the same column names but with different case so this means I couldn't use the same DB.mapping keys to consume both result sets. This pull request was the easiest thing I could come up with to let me use the same struct and DB.mapping for these stored procedures. I agree a better approach would support arbitrary key conversions.

I didn't even know DB::Serializable was a thing. I was reading the DB API docs and the mapping macro seemed to be how to do this since it is mentioned on the DB module page: https://crystal-lang.github.io/crystal-db/api/0.12.0/DB.html.

I'm going to close this pull request, and switch my project to use DB:Serializable.

Can the mapping macro be marked as deprecated? Also should we refresh the API docs to show how to use Serializable rather than the mapping macro on the DB module page, so other newbies don't make the same mistake I did?

@lachlan lachlan closed this Nov 8, 2023
@lachlan lachlan deleted the case_insensitive_mapping branch November 9, 2023 01:46
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.

4 participants