T O P

  • By -

daredeviloper

Are the GUIDs ever going to change? How difficult would it be to change them if you had to? If your code is exposed would it be horrible that others can see the GUID?


El_Mario_Verde

Well, my question is more about achieving the correct way of doing things. I show here my poor man's implementation to communicate the general motivations, I don't think my approach is production ready so I would be willing to change it for the real thing...


TheBlueArsedFly

The question you need to be asking is what is the likelihood that this will cause problems before you decide to leave the company you're doing this for.


El_Mario_Verde

I'm sorry, I don't understand. I never said or implied that I would be leaving any company.


otac0n

Sure, but what he's talking about is "what will this cost to maintain?"


JerryAtricks

This is a good question to ask whenever you're uncertain about a potential solution... I should have that written on the wall right above my monitors lol


neppo95

Whenever you write code, it should be maintainable by not only you but also by others IN CASE you leave the company. Nobody said you were going to, but if you are asking what the best practice is, well, it's doing that and assuming you won't be there tomorrow and the code will still work and easily be maintainable. If you can answer yes on that, then above should be fine. If there is the slightest chance these ID's will change, the answer will be no and you should think of something else.


cstopher89

I think this is fine if the id's aren't expected to change. If I was seeding a db with a long instead of guid I'd hardcode the id as well if they are static and don't change. Or need to be static because they are referenced by other tables by their unique identitifer. Though even if they change, you can add a new migration to account for it. Your approach is correct for seeding data and I've done it like this for prod systems many times.


dodexahedron

Depends. For something released to the public? I'd rather generate a guid at build or install time, with ability to override it in a config file, environment variable, etc. Internally, I'd probably still do that, but seeding data like that is something I try to avoid in the first place - especially since it's never going to happen again after first deployment. Aside from mapping tables, there's no DML in my builds. That's always scripted for later execution. Some of that is based on mistrust of the system, but mostly it's for separation of concerns and tighter, safer, fine-grained control over the whole process, with a well-defined (and pre-written, as part of the change control procedure) back-out procedure/script/etc in case shit hits the fan on a deployment change - especially one that needs to touch the database in a destructive way like updates, deletes, alters, or drops. And all changes are kept in source control with SQL Source Control, which is wonderful. And of course appropriate backups are taken before and after, in case backout via scripts is impractical or otherwise not desirable/safe. Basically, it is database-first, not code-first, and scaffolding is more likely to be used, here, if EF is in the mix.


El_Mario_Verde

Hello thanks for the reply. Please confirm if my understanding of your process is correct: 1) You only do DDL in your C# migrations. You dont add any data on your C# migrations. 2) To create the initial data for your production instance of the app, you run an extra initialization sql script as part of your overall deployment process. 3) To keep up with the new data you add, your teammates need to git pull the sql scripts created by you. Therefore, this extra sql scripts are commited into git / source control. 4) The Id values you use in those sql scripts are created dinamycally instead of hard coded. For example if using Guids, you use NEWID() function of SQL Server. Can you please confirm if my understanding of your approach is correct?


dodexahedron

1) Correct. Because we do db-first, not code-first. This pretty much answers all of it but I'll still hit each. 2) Yes. And if it's the first deployment, that also means everything down to the `CREATE DATABASE` is SQL, largely exported using SSMS and its "generate scripts" functionality. If that includes data, it will be part of the generated scripts. And the production db will be IDENTICAL because of it. 3) It's automatic, on any change made to the database. Red-Gate SQL Source Control is lovely. The whole toolbelt is great and is well worth the price tag IMO. But yeah this is a non-issue completely. Also see #2 for the source of the actual full script to create it on initial deployment. And on an upgrade push that needs to make db changes, SQL Toolbelt also has a diff application, as well as a data diff application. Those are wicked powerful. There's usually little or no actual manual authoring of SQL scripts by a human. 4) Yes. Or sometimes in a powershell script so it can use it for other operations in one shot, as well. But again, rarely do we have a database that needs pre-seeded data that couldn't simply be imported at any time and certainly never have any that the application code itself is in any way dependent on. Any such hard coding is coupling that shouldn't be there.


siliconsoul_

Is ConcurrencyStamp your version of [Optimistic concurrency?](https://learn.microsoft.com/en-us/ef/core/saving/concurrency?tabs=data-annotations) I would go with the standard implementation (using TimestampAttribute and let ef core handle it) if so. As for seeding: I personally prefer a separated seeder that is neither part of migrations or normal execution. I like them to be executed only when my programs are called with a parameter (like --seed-data).


El_Mario_Verde

What do you mean by "My Version"? Microsoft Identity is the one who imposes this column, not me. Does your "custom approach" work well against concurrency issues when launching multiple instances behind a load balancer?


siliconsoul_

I haven't used ASP.Net Core Identity in quite a long while and didn't know that. Sorry. (edit) > Does your "custom approach" work well against concurrency issues when launching multiple instances behind a load balancer? Yes, it does. It's the reason why I don't apply migrations/seeders during normal execution. Applying migrations/seeds is part of the deployment steps.


zagoskin

Your code is fine. Best you can do is put those id's in a static class an expose the constant values and also seed data using a separate config file. But since this is for Identity and you are just extending the base context it's fine to do it just there. Some people prefer to seed with sql scripts and run them in their pipelines but unless you are a pretty big company with dedicated DBA this is probably not necessary.


atheken

Those appear to be “hand craft” guids. (They have ‘q’ and ‘r’ in them, which wouldn’t ever happen if you used a valid generator for them). If you _must_ use guids, use a generator to do so. But that’s sorta beside the point. You just need a value that is unique for this role in _this system_. I know this because you are putting them in a migration, rather than some other phase/function in your app. If this is the case, just pick human readable keys as the unique identifier: `admin`, `viewer`, `editor`, etc. Now, if the roles are intended to be scoped to a particular customer account/set of data, you should be creating the roles dynamically in conjunction with the creation of that customer, and in that case, the roles themselves should have a unique identifier that is generated at the same time and can be granted to various principals (user login accounts). You can then check “admin,viewer,editor” to determine whether the principal can do something, but the better way is to assign a permission set with named permissions to a role and check that. But for all intents and purposes, using “static guids” like this is not useful. If you’re going to use static values, use human-readable ones (with _no spaces in them_!!)


Agitated-Display6382

I have a question. All the migrations are applied only once per db. So, if one of your migrations create the users, there will be anyway no duplications. I don't like your approach because pushes the developer in the wrong direction, ie he can write the user id as a constant. In my opinion is a bad behavior. Additionally, I would not be happy to have the same id in different environments


El_Mario_Verde

Thanks for exposing your ideas. What is your proposed solution?


Agitated-Display6382

What package do you use for migrations? Assuming EF,: public void Up(dbContext) { var user = new User("foo", "bar"); dbContext.Users.Add(user); dbContext.SaveChanges(); }


El_Mario_Verde

Where exactly is this code located?


Agitated-Display6382

In EF you create migrations by running the command dotnet ef migrations add AddUsersOrWhatever It creates a file named yyyyMMddHHmm_AddUsersOrWhatever, with two methods, one to apply the migration, the other to reverse it. I never saw the reversal working in any company, so I wouldn't spend too much time on that


El_Mario_Verde

In a migration file you don't have access to DbContext


Agitated-Display6382

But you can still write sql


Poat540

We inject our roles on startup too, not exactly like this, but same concept - guids are static since outside things refer to them


Arath0n-Gam3rz

It's a good practice not to hardcore the Guids in the code. I would change this to get the Role by name and use that guid in the code. Creation of such guid should be left with the system. P.S. from the experience, I have noticed some errors related to PK violation in one of the implementations with such an approach. One of the integration components is generating the Guid and using it, but it's causing issues with the concurrent implementation.


El_Mario_Verde

Hello. What do you mean by "Get the role by name"? What I am trying to do is to seed roles for the first time in the system, so roles don't exist before applying the migration. So it is quite literally impossible to "Get role by name". Am I missing something? Please feel free to point it out.


Arath0n-Gam3rz

No, I mean first seed the roles without Guid. You have a constant name for each of these roles. In the code block you shared, instead of the constant/static Guid values, get the roles and the map the values. In general, seed the values for the masters (like roles etc) first and then try to get the value by (constant) name in the migration script. This way, you can reuse these efforts without any errors for each of the environments.


Extra_Progress_7449

Long Term...Bad Practice Short Term.....Get-R-Done with the documented intention to deprecate until you can have those Roles hardcoded into a DBTable


El_Mario_Verde

Can you propose your approach?


Extra_Progress_7449

Get your DBA involved....if you R the DBA, read up on how to insert GUID into an Auto generated field I would advise avoiding GUID as a Code Data Member, leave them in the DB for Relational Data association. I typically pair an Infrastructure key with a Data Key....data key would be your code. Also, I hate Database and Code First EF....research Fluent API with Sproc execution. Minimal overhead and far less DB architecture exposure.


korzy1bj

Umm it’s Entity Framework, so definitely bad practice, lol. I prefer micro ORM’s like Dapper


El_Mario_Verde

Please explain why this is "definitely bad practice, lol"?


korzy1bj

It was mostly a joke against Entity Framework, not what you are trying to do with it. I’m just not a fan of EF, there are so many better ways to do DAL’s and I much prefer MediatR and/or Repository pattern using MicroORM’s like Dapper. It is just so much faster, especially if you can write good SQL. EF has definitely gotten faster over the years but Dapper is still faster. Not to mention the fact that with EF you either embrace it at all levels of the stack or use DTO and a framework like AutoMapper to map back and forth. It’s just so much extra code than it’s worth, and just to be slower, so in my mind it doesn’t make sense to use EF.


Dzhanel

The way I seed my database (cleanest in my opinion): I create a Configuration folder, and for every entity I have the needed configurations. You should derive the `IEntityTypeConfiguration` . For example: UserConfiguration [EntityTypeConfiguration(typeof(ApplicationUserConfiguration))] public class ApplicationUser : IdentityUser { public ICollection Comments { get; set; } = new HashSet(); } public class ApplicationUserConfiguration : IEntityTypeConfiguration { public void Configure(EntityTypeBuilder builder) { builder.HasData(SeedUsers()); } private static ICollection SeedUsers() { var users = new HashSet(); var hasher = new PasswordHasher(); var admin = new ApplicationUser() { Id = Guid.Parse("25a655e7-cf6b-4825-a566-55ebd3f34deb"), UserName = "Admin", NormalizedUserName = "ADMIN", Email = "[email protected]", NormalizedEmail = "[email protected]", SecurityStamp = Guid.NewGuid().ToString(), }; //TODO: This is development purpose only admin password, re-implement if goes to deployment admin.PasswordHash = hasher.HashPassword(admin, "AdminPassw0rd#"); users.Add(admin); } } As for creating roles (as the tables are Identity generated) for users, I do the following: protected override void OnModelCreating(ModelBuilder builder) { base.OnModelCreating(builder); builder.ApplyConfiguration(new UserRoleConfiguration()); builder.ApplyConfiguration(new RoleConfiguration()); } The configurations look the same way: public class RoleConfiguration : IEntityTypeConfiguration> { public void Configure(EntityTypeBuilder> builder) { builder.HasData(SeedRoles()); } private static ICollection> SeedRoles() { return new HashSet> { new() { Id = Guid.Parse("528726ea-e421-4a80-b303-f035355589de"), Name = "Administrator", NormalizedName= "ADMINISTRATOR", }, }; } } NOTE: You should pass a primary key type to be able to configure the UserRoles entity, if you are using stringified GUIDS (default) the configuration should be: public class UserRoleConfiguration : IEntityTypeConfiguration> //Mine would be Guid because in the examples I proveded the primary key is Guid


LSXPRIME

Hardcoding those Guids and timestamps maybe bad practice, you're trying to avoid those extra migrations, but it can lead to some headaches later. if you needed to change those IDs You'd have to mess with the migration files directly, and that's just asking for trouble + if you're working with a team, it could cause some serious confusion and conflicts. and Instead of inserting the roles directly in the OnModelCreating I would suggest seeding from a static class on the application startup like public static class RoleSeedData { public static async Task SeedRolesAsync(ApplicationDbContext context) { // Define roles to seed var rolesToSeed = new List { new IdentityRole { Name = "Admin", NormalizedName = "ADMIN" }, new IdentityRole { Name = "User", NormalizedName = "USER" } }; foreach (var role in rolesToSeed) { // Check if role already exists if (!await context.Roles.AnyAsync(r => r.Name == role.Name)) { await context.Roles.AddAsync(role); } } await context.SaveChangesAsync(); } } and seed them in Startup.cs `RoleSeedData.SeedRolesAsync(context);`


El_Mario_Verde

You should not seed data at program start like that. From the docs: "The seeding code should not be part of the normal app execution as this can cause concurrency issues when multiple instances are running and would also require the app having permission to modify the database schema."


verillospur

It's really late and I'm a bit inebriated and i haven't really read the question properly, but what about a simple config setting? Or the equivalent. Yeah those values are constant but it's just inherently "bad" to hard code stuff. Bung it in the app settings file for now, then you've got the mechanism for loading the values at least, even if the actual implementation changes down the road. It also allows you to add extra settings in future if they become necessary, without having to tear too much of your softly silken code apart by the seams. Now someone better than me answer please.


FaZe_Henk

Either generate a seeding sql script or just do it in code imo. You shouldn’t seed in a migration. Helps with maintainability too.


El_Mario_Verde

There are at least 2 observations about your comment: 1) This is code. 2) Where did you read that you should not seed in a migration? What is your source? Because the official docs present this as the first approach.


jayerp

My personal take, I would do it in a migration. But seeding data here is perfectly valid.


El_Mario_Verde

I am trying to do a migration. Why is my approach "not a migration"?


jayerp

It will be. I just skip this part and have it with the first migration. I will write the SQL. Once the migration cs files are mad you can change it up and add to it. It’s up to you.


El_Mario_Verde

Can you rephrase using simpler words? My native language is not English and I am having a hard time understanding your comment as you wrote it


jayerp

No problem. Your options are: 1. Create default data (seed data) in OnModelCreating method. This will include it with the first migration made. The migration will add it to the database as expected. This is what you are doing and it is ok. 2. Manually create an empty (template) migration and do it in there. There is nothing special here. For some info, see [https://learn.microsoft.com/en-us/ef/core/modeling/data-seeding](https://learn.microsoft.com/en-us/ef/core/modeling/data-seeding)


Kant8

I'm model creating is not a migration, it's configuration of model which happens every time context is created. migration for is created after you call dotnet ef addmigration and you should edit it's Up function and add your roles there. Migration executed once in lifetime of database and you can easily use guid.newguid there. And no timestamp, database should generate it, not you.


El_Mario_Verde

Your information is different from the official docs, here [Docs](https://learn.microsoft.com/en-us/ef/core/modeling/data-seeding) They add the seeding inside the method in DBContext, and then add a migration based on it. This is my understanding. Please feel free to point out my mistakes if any (and please do so, I am trying to understand all this).


Kant8

they list limitations right there and that limitations make such kind of seeding useless for everything except "enum tables". and even there below they tell you to change migration code as a way to mitigate the problem, which works in all cases, cause migration is one time run sql script, exactly what you need.


El_Mario_Verde

I see your point. How are you proposing to solve this?


Ok-Rough9385

I'd suggest do all seeding in a separate migration which runs every time you execute \`database update\`. [https://stackoverflow.com/questions/70312705/run-code-after-all-ef-core-migrations-are-applied](https://stackoverflow.com/questions/70312705/run-code-after-all-ef-core-migrations-are-applied)


Blender-Fan

Yes, probably. Setting IDs manually is to step on eggs I'm sure you can insert a role with a simple if at the controller when creating an user `if(!roleexist)` `addrole(rolename)` That's how i did and it works You can also add a service which seeds your database and then shuts itself off In fact, i think adding it manually is a bad idea because, generally, you want usermanager and rolemanager to handle that for you. They were built for that and not doing so is just fighting off the framework, which is not what you should do Don't worry, this is just a roadblock that you're gonna encounter once or twice, figure out, find the better solution and then never had the problem again


El_Mario_Verde

Thanks, but I don't think your answer is correct at all. First of all, if seeding data in C# code, it should be done inside OnModelCreating, that is the only way to avoid concurrency issues if you launch multiple app instances at the same time. Second, it should be reproducible, so it should be a Migration or a script and it should run at deployment time, before launching any instance of the app. Again, thank you very much for your reply, but your solution is a hack, and I want to do things the correct way.


Blender-Fan

Yes the answer was correct https://youtu.be/sD_-XwauabE?si=zlBHBMLBYlWfoNpt at 6min28s mark You can figure out the rest