OrchardCore
OrchardCore copied to clipboard
Validate database prefix before allowing an admin to be added
- Fix #11803
- Fix #11863
- Fix #11876
Thanks again Mike, seems still you have a Git issue ;)
I may share simpler way to fix the issue, let me try it tonight
Thanks again Mike, seems still you have a Git issue ;)
Yes I see that. Please check #11825
I may share simpler way to fix the issue, let me try it tonight
Can't wait to see a simpler way.
Thank you
If the pr #11713 merged, the code in ConnectionValidator
should get updated by injecting ITableNameConvention in the constructor and using it instead of manually creating an instance during validation
Please HOLD ON, I'm working on similar PR to make it easier to achieve this goal, also we need more unit tests to make sure nothing broken
If my attempt failed probably I could add some modifiaction into this PR
FYI I labled this PR as dontmerge
because I mentioned at the beginning before this PR started that I have a simpler way to achieve the goal
After exchanging messages and voice notes with @CrestApps I realize there are two issues:
- Avoid adding duplicate table prefix in the same database
- Checking table existance (or check if Documents table is there) to avoid invali serial number issue
Based on that I created the PR #11846 which is fixes the issue without hitting the database. Backing to this PR I see the connection validations is good for certain scenarios such as checking Documents table, database is LIVE. So, if we add the validation during tenant creation that we be a behavior breaking-change because in the past the user can create a tenant without the database exists.
I suggest to :
- Open another issue explaining point 2, so this PR can be modified little bit and fine to fix the issue
- The PR I created can target the old issue because as described above it should avoid adding a duplicate table prefix on the same database.
- Add connection validation if the provider has connectionString (If we agreed that we can accept the behavior breaking changes)
/cc @sebastienros @jtkech @Skrypt @agriffard yours feedback is very important here
The build is broken :(
The build is broken :(
Very strange the build failed to pull the required packages which is why it failed. I faked a commit to force it to build again
I will make a quick review, but can your PR target the issue no. 2 to make it clear what each PR is target
I will make a quick review, but can your PR target the issue no. 2 to make it clear what each PR is target
@hishamco I created another issue to describe the other scenarios
@CrestApps can this target #11863 and related to the prefix issue, coz invalid serial could be happen if we didn't specify a table prefix
@sebastienros hope we can accept this PR to resolve the invalid serial issue, but again I see table prefix issue can be easily resolved using settings
Fix conflicts by merging latest main branch.
@Skrypt done.
This one for next triage