Provide option for alternative form of delete link request Uri #2209

gathogojr · 2021-09-30T20:23:08Z

Issues

This pull request fixes #2201.

Description

Provide option for form of delete link request Uri.

According to the OData spec, a delete link request should have a Uri like:

{ServiceUri}/{EntitySet}/{Key}/{NavigationProperty}/$ref?$id={ServiceUri}/{RelatedEntitySet}/{RelatedKey}

But in V3 and V4.01, the following alternative form is supported:

{ServiceUri}/{EntitySet}/{Key}/{NavigationProperty}/{RelatedKey}/$ref

Checklist (Uncheck if it is not completed)

Test cases added
Build and test with one-click build and test script passed

habbes · 2021-10-05T09:25:38Z

src/Microsoft.OData.Client/BaseSaveResult.cs

+            {
+                // Related key segment should appear before /$ref
+                int indexOfLinkSegment = uriString.IndexOf(XmlConstants.UriLinkSegment, StringComparison.Ordinal);
+                builder.Append(indexOfLinkSegment > 0 ? uriString.Substring(0, indexOfLinkSegment - 1) : uriString);


why not >= 0 since 0 can be a valid index?

Maybe we can use Span and slice the string to avoid allocation with Substring

@habbes Using >= 0 would suggest that uriString starts with $ref which is not practical in this case. So while we can make that change, it's really not a practical scenario

@habbes Span (and ReadOnlySpan) are not available in NET45 but we could introduce the System.Memory package to take care of that. Maybe to keep the change small we could live with that single extra allocation since this code is not on a hot path

habbes · 2021-10-05T09:42:00Z

src/Microsoft.OData.Client/DeleteLinkUriOption.cs

+    /// <summary>
+    /// Used to specify the form of Uri to be used for a delete link request.
+    /// </summary>
+    public enum DeleteLinkUriOption


Out of curiosity, does this issue with different URI options only affect DELETE requests?

It only affects DELETE requests

habbes · 2021-10-05T09:44:55Z

src/Microsoft.OData.Client/DeleteLinkUriOption.cs

+        /// Pass the id of the related entity as a query param, i.e.,
+        /// {ServiceUri}/{EntitySet}/{Key}/{NavigationProperty}/$ref?$id={ServiceUri}/{RelatedEntitySet}/{RelatedKey}
+        /// </summary>
+        DollarIdQueryParam = 0,


I'd suggest the name to be IdQueryParam. In case we have situtations where the $ is optional.

$ sign is only optional in system query options e.g select, top, expand etc

KenitoInc · 2021-10-05T09:52:31Z

src/Microsoft.OData.Client/DeleteLinkUriOption.cs

+        /// Pass the id of the related entity as a query param, i.e.,
+        /// {ServiceUri}/{EntitySet}/{Key}/{NavigationProperty}/$ref?$id={ServiceUri}/{RelatedEntitySet}/{RelatedKey}
+        /// </summary>
+        DollarIdQueryParam = 0,


$ sign is only optional in system query options e.g select, top, expand etc

xuzhg

mikepizzo

Please create an issue to consider changing the default in the next breaking change release.

pull-request-quantifier-deprecated · 2021-10-07T19:55:27Z

This PR has 140 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details

Label      : Medium
Size       : +137 -3
Percentile : 48%

Total files changed: 5

Change summary by file extension:
.cs : +136 -3
.csproj : +1 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

Fast and predictable releases to production:
- Optimal size changes are more likely to be reviewed faster with fewer
  iterations.
- Similarity in low PR complexity drives similar review times.
Review quality is likely higher as complexity is lower:
- Bugs are more likely to be detected.
- Code inconsistencies are more likely to be detetcted.
Knowledge sharing is improved within the participants:
- Small portions can be assimilated better.
Better engineering practices are exercised:
- Solving big problems by dividing them in well contained, smaller problems.
- Exercising separation of concerns within the code changes.

What can I do to optimize my changes

Use the PullRequestQuantifier to quantify your PR accurately
- Create a context profile for your repo using the context generator
- Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
- Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
- Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
Change your engineering behaviors
- For PRs that fall outside of the desired spectrum, review the details and check if:
  - Your PR could be split in smaller, self-contained PRs instead
  - Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

One line was added: +1 -0
One line was deleted: +0 -1
One line was modified: +1 -1 (git diff doesn't know about modified, it will
interpret that line like one addition plus one deletion)
Change percentiles: Change characteristics (addition, deletion, modification)
of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

pull-request-quantifier-deprecated bot added the Extra Large label Sep 30, 2021

gathogojr changed the title ~~Provide option for alternative forms of delete link request Uri~~ Provide option for alternative form of delete link request Uri Sep 30, 2021

gathogojr force-pushed the feature/2201-provide-option-for-delete-link-uri branch from 75cc304 to 89a5ff4 Compare September 30, 2021 20:33

pull-request-quantifier-deprecated bot added Extra Small and removed Extra Large labels Sep 30, 2021

gathogojr force-pushed the feature/2201-provide-option-for-delete-link-uri branch from 89a5ff4 to 94b6444 Compare October 4, 2021 13:55

pull-request-quantifier-deprecated bot added Medium and removed Extra Small labels Oct 4, 2021

gathogojr requested review from ElizabethOkerio, habbes, KenitoInc, marabooy, mikepizzo and xuzhg October 4, 2021 13:56

marabooy approved these changes Oct 5, 2021

View reviewed changes

habbes approved these changes Oct 5, 2021

View reviewed changes

KenitoInc approved these changes Oct 5, 2021

View reviewed changes

xuzhg approved these changes Oct 5, 2021

View reviewed changes

mikepizzo approved these changes Oct 5, 2021

View reviewed changes

gathogojr force-pushed the feature/2201-provide-option-for-delete-link-uri branch from 94b6444 to f43ea3a Compare October 7, 2021 19:48

Provide option for alternative form of delete link request Uri

b0cf008

gathogojr force-pushed the feature/2201-provide-option-for-delete-link-uri branch from f43ea3a to b0cf008 Compare October 7, 2021 19:55

gathogojr merged commit ffd2459 into OData:master Oct 8, 2021

gathogojr deleted the feature/2201-provide-option-for-delete-link-uri branch October 8, 2021 06:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide option for alternative form of delete link request Uri #2209

Provide option for alternative form of delete link request Uri #2209

gathogojr commented Sep 30, 2021 •

edited

Loading

habbes Oct 5, 2021

habbes Oct 5, 2021

gathogojr Oct 7, 2021 •

edited

Loading

gathogojr Oct 7, 2021

habbes Oct 5, 2021

gathogojr Oct 7, 2021 •

edited

Loading

habbes Oct 5, 2021

KenitoInc Oct 5, 2021

KenitoInc Oct 5, 2021

xuzhg left a comment

mikepizzo left a comment

pull-request-quantifier-deprecated bot commented Oct 7, 2021

What can I do to optimize my changes

How to interpret the change counts in git diff output

Provide option for alternative form of delete link request Uri #2209

Provide option for alternative form of delete link request Uri #2209

Conversation

gathogojr commented Sep 30, 2021 • edited Loading

Issues

Description

Checklist (Uncheck if it is not completed)

habbes Oct 5, 2021

Choose a reason for hiding this comment

habbes Oct 5, 2021

Choose a reason for hiding this comment

gathogojr Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

gathogojr Oct 7, 2021

Choose a reason for hiding this comment

habbes Oct 5, 2021

Choose a reason for hiding this comment

gathogojr Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

habbes Oct 5, 2021

Choose a reason for hiding this comment

KenitoInc Oct 5, 2021

Choose a reason for hiding this comment

KenitoInc Oct 5, 2021

Choose a reason for hiding this comment

xuzhg left a comment

Choose a reason for hiding this comment

mikepizzo left a comment

Choose a reason for hiding this comment

pull-request-quantifier-deprecated bot commented Oct 7, 2021

What can I do to optimize my changes

How to interpret the change counts in git diff output

gathogojr commented Sep 30, 2021 •

edited

Loading

gathogojr Oct 7, 2021 •

edited

Loading

gathogojr Oct 7, 2021 •

edited

Loading