Skip to content
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

Update change-notifications-subscriptions-example-renewal-java-snippets.md #9498

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Yusvanth
Copy link

@Yusvanth Yusvanth commented Jan 20, 2025

NotificationUrl is required in new subscription object which is used in patch request to renew a subscription.


Description :

Along with ExpirationDateTime, NotificationUrl is also required in new subscription object which is used in patch request to renew a subscription.
When a patch request is made without a notificationUrl in subscription object, the patch request gets failed with message :

Subscription must contain either the 'expirationDateTime' or 'notificationUrl' property and its value.

Although the message states that the subscription must include either 'expirationDateTime' or 'notificationUrl', the patch request succeeds only when both fields are populated in the subscription object. This issue occurs when using the Java SDK for the request, but not when using curl or Postman.


Important

The following guidance is for Microsoft employees only. Community contributors can ignore this message; our content team will manage the status.

After you've created your PR, expand this section for tips and additional instructions.
  • do not merge is the default PR status and is automatically added to all open PRs that don't have the ready to merge label.
  • Add the ready for content review label to start a review. Only PRs that have met the minimum requirements for content review and have this label are reviewed.
  • If your content reviewer requests changes, review the feedback and address accordingly as soon as possible to keep your pull request moving forward. After you address the feedback, remove the changes requested label, add the review feedback addressed label, and select the Re-request review icon next to the content reviewer's alias. If you can't add labels, add a comment with #feedback-addressed to the pull request.
  • After the content review is complete, your reviewer will add the content review complete label. When the updates in this PR are ready for external customers to use, replace the do not merge label with ready to merge and the PR will be merged within 24 working hours.
  • Pull requests that are inactive for more than 6 weeks will be automatically closed. Before that, you receive reminders at 2 weeks, 4 weeks, and 6 weeks. If you still need the PR, you can reopen or recreate the request.

For more information, see the Content review process summary.

…ts.md

Along with ExpirationDateTime, NotificationUrl is also required in new subscription object which is used in patch to renew a subscription.
Copy link

Learn Build status updates of commit 4117e27:

✅ Validation status: passed

File Status Preview URL Details
includes/snippets/java/v1/change-notifications-subscriptions-example-renewal-java-snippets.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

@FaithOmbongi FaithOmbongi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Yusvanth - The code snippets are all autogenerated based on the HTTP request block. Any manual fixes will be overridden the following week.

Please apply the change to the HTTP request example that references this snippet and I'll confirm with engineering before approving and merging the changes. Thank you.

@FaithOmbongi FaithOmbongi self-assigned this Jan 22, 2025
@FaithOmbongi FaithOmbongi added area: change notifications Issues and PRs related to subscriptions, change notifications, webhooks, and lifecycle events. area: Entra APIs and issues relating to Microsoft Entra (previously Azure AD) features. changes requested labels Jan 22, 2025
Copy link

Learn Build status updates of commit 008b108:

✅ Validation status: passed

File Status Preview URL Details
includes/snippets/java/v1/change-notifications-subscriptions-example-renewal-java-snippets.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

…bscriptions-example-renewal-java-snippets.md
Updated change-notifications-delivery-webhooks.md file to resolve the issue exits in subscription patch request .

Without notificationUrl the patch request fails with message " Subscription must contain either the 'expirationDateTime' or 'notificationUrl' property and its value. "
Copy link

Learn Build status updates of commit b64fbaa:

✅ Validation status: passed

File Status Preview URL Details
includes/snippets/java/v1/change-notifications-subscriptions-example-renewal-java-snippets.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

Copy link

Learn Build status updates of commit 79dd493:

✅ Validation status: passed

File Status Preview URL Details
concepts/change-notifications-delivery-webhooks.md ✅Succeeded
includes/snippets/java/v1/change-notifications-subscriptions-example-renewal-java-snippets.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

@Yusvanth
Copy link
Author

Yusvanth commented Jan 22, 2025 via email

@Yusvanth
Copy link
Author

Hi @FaithOmbongi ,

Thank you for your feedback. I’ve updated the HTTP request example to address the requested changes and ensure it aligns with the snippet. Please review and confirm if it meets the engineering requirements. Let me know if any further adjustments are needed.

As a gentle reminder, kindly convey to the engineering team that this issue cannot be reproduced when making an HTTP request as specified in the existing code. The issue appears to exist only in the SDK call.

Looking forward to your confirmation.

@Yusvanth
Copy link
Author

#feedback-addressed

@FaithOmbongi
Copy link
Contributor

Hi @Yusvanth - Thank you for this additional context 👇🏾 - this changes the troubleshooting channels, unfortunately. Because the issue is no longer a doc issue. From your comment, the issue might not be in the API as documented but might be an SDK-related issue.

As a gentle reminder, kindly convey to the engineering team that this issue cannot be reproduced when making an HTTP request as specified in the existing code. The issue appears to exist only in the SDK call.

Unfortunately, this repo isn't for investigating product issues. Could you file a support request via Q&A for further investigation? The bug will be escalated through the various channels. I'll keep this PR open as we wait for feedback from the support engineers.

@FaithOmbongi FaithOmbongi added awaiting internal response needs author feedback Waiting for author (creator) of issue to provide more info and removed review feedback addressed labels Jan 23, 2025
@Yusvanth
Copy link
Author

Thank you @FaithOmbongi for the information, I have filed a support request via Q&A.

I do have a humble request, if this isn't a SDK related issue, lets say 'notificationUrl' is actually needed in SDK to perform the business logic, in this case can we mention this in the documentation, I would be happy to contribute, I am sure it would be very useful for developers, I actually found this as a production bug, thats why I think its necessary.

Also I kindly request the engineering team to verify if this is an issue in other languages as well, i encountered this in Java SDK, not sure with other languages.

@FaithOmbongi
Copy link
Contributor

I do have a humble request, if this isn't a SDK related issue, lets say 'notificationUrl' is actually needed in SDK to perform the business logic, in this case can we mention this in the documentation, I would be happy to contribute, I am sure it would be very useful for developers, I actually found this as a production bug, thats why I think its necessary.

We greatly appreciate your contribution @Yusvanth. It is a pleasure to receive your input and resolve documentation bugs, which helps other devs.

If notificationUrl property is confirmed as Required and not Optional as currently documented, I will gladly approve and merge this PR. In addition, if confirmed, I will request that you also update the example in the beta and v1.0 reference docs to complete the circle of required updates.

I'm tracking the bug you filed in Q&A and will be on the lookout for feedback from support engineers so we can action on this PR.

@Yusvanth
Copy link
Author

Thank you, @FaithOmbongi, for your prompt response and support. I'll await for feedback from the engineering team and proceed with the necessary updates once clarified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: change notifications Issues and PRs related to subscriptions, change notifications, webhooks, and lifecycle events. area: Entra APIs and issues relating to Microsoft Entra (previously Azure AD) features. awaiting internal response changes requested needs author feedback Waiting for author (creator) of issue to provide more info
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants