-
Notifications
You must be signed in to change notification settings - Fork 601
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
Feat: Add the ability to use schema names for tables in table names #572
base: main
Are you sure you want to change the base?
Feat: Add the ability to use schema names for tables in table names #572
Conversation
8c818fe
to
7681bd7
Compare
Hi, @NickCraver Could you take a look? And why did yoy replace |
adc9d55
to
8758fd7
Compare
Hey there! First, thanks for willingness to contribute here :) Can I zoom out a bit and figure out what problem we're trying to solve with the changes here? There are a few issues with breaking constructors and such, but I'd like to understand what's not working today. It's supposed to be handled intentionally as your table name could for example be There is a problem always committing schema, and that'd break some use cases I've personally maintained. In some situations, you want to use "my schema's table", whatever schema that login may default to (this happens in multi-tenancy environments). In that vein, it was an intentional decision to not bake schema into the queries here. That's just for context, but let's try and solve whatever problem let you here! |
If we specify In fact, the constructor does not break, since all previous use cases remain valid. I can correct the changes I added so that when specifying I also added a check for the existence of the created tables, since an exception occurred if they were already created. |
@NickCraver |
@NickCraver Could you take a look at my little fixes? P.S. Maybe should use |
Hey @unchase - busy week here. I believe the solution here remains much more complicated than is needed to solve the problem. We can add brackets on table names for example. The schema change is a breaking change, all optional parameter additions are binary breaking changes (as consumers will get a missing method exception). Given you're the first person I'm aware of hitting this, can you expound on the use case? For example, have you considered changing the default schema on the user MiniProfiler logs in with instead? |
Hmm. There are optional parameters, so how consumers will get a missing method exception? And this is not a breaking change, since all old calls to the constructor will also be valid (since they contain either only But I agree with you, more investigation needs to be done. The database used contains several tables for different purposes (Hangfire, MiniProfiler, and some application tables with separated schemes: And I would like to divide access to certain tables by schemas. Therefore, changing the default schema for the user is not appropriate. |
The way optional parameters are implemented in .NET, the caller has the values. So any assembly compiled against this one is looking for exactly the current method signature. For example let's say we have: public void Foo(string a) A caller is looking for public void Foo(string a, string b = null) Then a caller with the same single arg is now looking for Does that help convey why it's a breaking change? You're not the first person to hit this confusion, I see it quite often and probably should write a better blog post and example to point to. I'll try and poke at this for alternatives, just be aware that I stepped away from Stack after 10 years just yesterday and do plan on taking it easy at least a few days before doing much again :) |
Yes, now it became clear to me, thanks for the clarification. |
6669e92
to
f9074fb
Compare
@NickCraver I added a few changes to ensure backward compatibility (no breaking changes now I think). |
f9074fb
to
30d049e
Compare
Hey @unchase, sorry I've been busy lately - hadn't been announced I was starting at Microsoft when you filed this :) I think your original approach of adding a non-breaking overload with schema name is the way to go here (rather than battling an infinite number of SQL syntaxes with Regex). However, I don't think we should create schemas - that's not really the job of a library, that's a DBA responsibility and in many environments we'll have permissions in the schema we're given but not the ability to create a schema - so there's a scoping issue of where we can operate if introducing such a request. In the table creation bits, the fact we're not checking if the tables are created already (and failing) is very much by design - we don't want to generate tables left and right on a mis-config, it's intended to be a very intentional on-setup operation from the caller. Happy to help tweak here or can implement off base, but given the number of changes in play I think it may be much simpler to go from off I do appreciate you exploring the simplified approach to the index conflict, but with the extent needed to make it work, it's much more obvious your first approach is far more reasonable and maintainable - that's my fault. Happy to help remedy. |
Allows to define database schema name for tables used by MiniProfiler.
For example:
P.S. Introduced in
DatabaseStorageBase
, so far implemented only inSqlServerStorage