CSS-9587 unique offer urls #1273
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
babakks
approved these changes
Jul 17, 2024
Comment on lines
+75
to
+76
| err = j.Database.GetApplicationOffer(ctx, &offerCheck) | ||
| if err == nil { |
There was a problem hiding this comment.
suggestion: Since err is just used in the if-else statement, you can limit the scope by doing if err := ...; err == nil {,
pkulik0
approved these changes
Jul 17, 2024
| if err == nil { | ||
| return errors.E(fmt.Sprintf("offer %s already exists, use a different name", offerURL.String()), errors.CodeAlreadyExists) | ||
| } else { | ||
| if errors.ErrorCode(err) != errors.CodeNotFound { |
There was a problem hiding this comment.
a leftover after removing something? -> else if
feel free to leave it as is, only a semantic difference
kian99
force-pushed
the
CSS-9587-unique-offer-urls
branch
from
071858b to
bc94895
Compare
ale8k
reviewed
Jul 18, 2024
| @@ -0,0 +1,4 @@ | |||
| -- 1_11.sql is a migration that enforces uniqueness on URLs in application offers. | |||
| ALTER TABLE application_offers ADD UNIQUE (url); | |||
There was a problem hiding this comment.
I guess again can we have the gorm tag too plz
There was a problem hiding this comment.
Will add this in a follow-up
ale8k
reviewed
Jul 18, 2024
| offerCheck.URL = offerURL.String() | ||
| err = j.Database.GetApplicationOffer(ctx, &offerCheck) | ||
| if err == nil { | ||
| return errors.E(fmt.Sprintf("offer %s already exists, use a different name", offerURL.String()), errors.CodeAlreadyExists) |
There was a problem hiding this comment.
please use a different offer name would be better language
There was a problem hiding this comment.
Will add this in a follow-up
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR builds on #1272. Review the second commit from this PR.
This change ensures uniqueness of offer URLs. Offer URLs are used by Juju clients when performing cross-model relations. It's important that offer URLs maintain uniqueness as we use the URL to query our DB for offer details. Currently it is possible for an offer URL to be duplicated if a user creates an offer for 2 models where the models have the same name and the offer name is the same.
Two models with the same name are possible when they are owned by separate users. The offer URL resembles
username/model-name.offer-name. To resolve this issue, add a unique constrain to the DB and simply check if the offer URL already exists before attempting to create it. If it does exist, return an error and request the user to use a different alias for the offer-name. By default when no offer-name alias is provided, the name will match the application being offered but the user can supply an alternative name.Fixes CSS-9587
Engineering checklist
Check only items that apply