RavenDB-22176 Exposing notifications via GET endpoint #18318

maciejaszyk · 2024-04-02T14:41:45Z

Issue link

https://issues.hibernatingrhinos.com/issue/RavenDB-22176

Additional description

Type of change

Bug fix
Regression bug fix
Optimization
New feature

How risky is the change?

Low
Moderate
High
Not relevant

Backward compatibility

Non breaking change
Ensured. Please explain how has it been implemented?
Breaking change
Not relevant

Is it platform specific issue?

Yes. Please list the affected platforms.
No

Documentation update

This change requires a documentation update. Please mark the issue on YouTrack using Documentation Required tag.
No documentation update is needed

Testing by Contributor

Tests have been added that prove the fix is effective or that the feature works
Internal classes added to the test class (e.g. entity or index definition classes) have the lowest possible access modifier (preferable private)
It has been verified by manual testing

Testing by RavenDB QA team

This change requires a special QA testing due to possible performance or resources usage implications (CPU, memory, IO). Please mark the issue on YouTrack using QA Required tag.
No special testing by RavenDB QA team is needed

Is there any existing behavior change of other features due to this change?

Yes. Please list the affected features/subsystems and provide appropriate explanation
No

UI work

It requires further work in the Studio. Please mark the issue on YouTrack using Studio Required tag.
No UI work is needed

Lwiel · 2024-04-03T10:51:24Z

src/Raven.Server/NotificationCenter/Handlers/DatabaseNotificationCenterHandler.cs


 namespace Raven.Server.NotificationCenter.Handlers
 {
    public class DatabaseNotificationCenterHandler : DatabaseRequestHandler
    {
+        [RavenAction("/databases/*/notification-center/get", "GET", AuthorizationStatus.ValidUser, EndpointType.Read, SkipUsagesCount = true)]


I'd change the path to /databases/*/notification-center/notifications or something similar

I don't like it

On the other hand /get matches other paths in this handler, so maybe it should stay like this

GET /databases/notifications

Lwiel · 2024-04-03T11:01:23Z

test/SlowTests/Issues/RavenDB_22176.cs

+public class RavenDB_22176 : RavenDB_17068
+{
+    [RavenFact(RavenTestCategory.Indexes)]
+    public async Task GetCurrentNotificationsAsJson()


Maybe something like TestGetNotificationsEndpoint would be more descriptive

arekpalinski · 2024-04-05T06:08:35Z

src/Raven.Server/NotificationCenter/Handlers/DatabaseNotificationCenterHandler.cs


 namespace Raven.Server.NotificationCenter.Handlers
 {
    public class DatabaseNotificationCenterHandler : DatabaseRequestHandler
    {
+        [RavenAction("/databases/*/notification-center/get", "GET", AuthorizationStatus.ValidUser, EndpointType.Read, SkipUsagesCount = true)]
+        public async Task GetNotifications()
+        {


Please provide parameter to determine type of notifications to get (alerts, performance hints)

arekpalinski · 2024-04-09T09:38:23Z

src/Raven.Server/NotificationCenter/Handlers/DatabaseNotificationCenterHandler.cs

+        {
+            var postponed = GetBoolValueQueryString("postponed", required: false) ?? true;
+            var type = GetStringQueryString("type", false);
+            var shouldFilter = type != null && Enum.TryParse(typeof(NotificationType), type, out _);


If we cannot parse we should throw instead of silently ignoring that

ppekrol · 2024-04-09T13:28:20Z

src/Raven.Server/NotificationCenter/Handlers/DatabaseNotificationCenterHandler.cs


 namespace Raven.Server.NotificationCenter.Handlers
 {
    public class DatabaseNotificationCenterHandler : DatabaseRequestHandler
    {
+        [RavenAction("/databases/*/notification-center/get", "GET", AuthorizationStatus.ValidUser, EndpointType.Read, SkipUsagesCount = true)]


GET /databases/notifications

src/Raven.Server/NotificationCenter/Handlers/DatabaseNotificationCenterHandler.cs

ppekrol · 2024-04-09T13:32:02Z

test/SlowTests/Issues/RavenDB-17068.cs

 {
-    public RavenDB_17068(ITestOutputHelper output) : base(output)
+    public RavenDb17068(ITestOutputHelper output) : base(output)


ppekrol · 2024-04-09T13:32:09Z

test/SlowTests/Issues/RavenDB-17068.cs

@@ -18,9 +18,9 @@

 namespace SlowTests.Issues;

-public class RavenDB_17068: RavenTestBase
+public sealed class RavenDb17068: RavenDB_17068_Base


can you fix the name, revert it

this will mess up with test history on CI

Unintentional, sorry!

ppekrol · 2024-04-09T13:34:59Z

src/Raven.Server/NotificationCenter/Handlers/DatabaseNotificationCenterHandler.cs

+                    isFirst = false;
+                }
+
+                writer.WriteEndArray();


I'm missing paging, would love to have sth like TotalCount or TotalResults and even in-memory paging here with optimization that pageSize=0 will just return count, like we have for regular queries

arekpalinski · 2024-04-10T13:27:28Z

src/Raven.Server/NotificationCenter/Handlers/DatabaseNotificationCenterHandler.cs

+            var pageSize = GetIntValueQueryString("pageSize", required: false) ?? int.MaxValue;
+
+            var shouldFilter = type != null;
+            if (shouldFilter && Enum.TryParse(typeof(NotificationType), type, out _) == false)


NotificationType enum is quite big:

ravendb/src/Raven.Server/NotificationCenter/Notifications/NotificationType.cs

Lines 3 to 13 in 3bd416c

public enum NotificationType

{

None,

AlertRaised,

OperationChanged,

DatabaseChanged,

NotificationUpdated,

RecentError, // used in studio

PerformanceHint,

DatabaseStatsChanged,

ClusterTopologyChanged

And the only relevant values here are AlertRaised and PerformanceHint, right? (because only those are persisted). I wonder if we should use this enum. Maybe better to have support for type=[alert|performance-hint]? @ppekrol?

I would prefer to go with enum, but you are correct, this one is not good. Can we introduce a new one? Maybe even with [Flags] attribute, so we can pick multiple ones?

…s for `RavenDB_17068` and `RavenDB_22176` tests to avoid repeating tests.

…persist test history in CI | Added the ability to retrieve notifications in a paging fashion.

maciejaszyk requested a review from Lwiel April 2, 2024 14:41

github-actions bot added the v5.4 label Apr 2, 2024

Lwiel reviewed Apr 3, 2024

View reviewed changes

maciejaszyk force-pushed the RavenDB-22176 branch from 42eba9e to d36797d Compare April 4, 2024 12:57

arekpalinski reviewed Apr 5, 2024

View reviewed changes

arekpalinski reviewed Apr 9, 2024

View reviewed changes

arekpalinski requested a review from ppekrol April 9, 2024 09:40

maciejaszyk force-pushed the RavenDB-22176 branch from e41f394 to cf019a0 Compare April 9, 2024 09:53

ppekrol reviewed Apr 9, 2024

View reviewed changes

arekpalinski reviewed Apr 10, 2024

View reviewed changes

maciejaszyk added 3 commits April 11, 2024 12:31

RavenDB-22176 Exposing notifications via GET endpoint

f559484

RavenDB-22176 Allow filtering notifications by type | Create baseclas…

6c52101

…s for `RavenDB_17068` and `RavenDB_22176` tests to avoid repeating tests.

RavenDB-22176 Adjusted endpoint name | Reverted class name change to …

b739a07

…persist test history in CI | Added the ability to retrieve notifications in a paging fashion.

maciejaszyk force-pushed the RavenDB-22176 branch from 01a5851 to 4852b50 Compare April 11, 2024 10:32

RavenDB-22176 Allow filter notifications with a mask, paging fix

4802944

maciejaszyk force-pushed the RavenDB-22176 branch from 4852b50 to 4802944 Compare April 11, 2024 10:33

arekpalinski approved these changes Apr 11, 2024

View reviewed changes

arekpalinski merged commit 26ded91 into ravendb:v5.4 Apr 11, 2024
15 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RavenDB-22176 Exposing notifications via GET endpoint #18318

RavenDB-22176 Exposing notifications via GET endpoint #18318

maciejaszyk commented Apr 2, 2024

Lwiel Apr 3, 2024

maciejaszyk Apr 9, 2024

Lwiel Apr 9, 2024

ppekrol Apr 9, 2024

Lwiel Apr 3, 2024

maciejaszyk Apr 9, 2024

arekpalinski Apr 5, 2024

maciejaszyk Apr 9, 2024

arekpalinski Apr 9, 2024

maciejaszyk Apr 9, 2024

ppekrol Apr 9, 2024

ppekrol Apr 9, 2024

ppekrol Apr 9, 2024

ppekrol Apr 9, 2024

maciejaszyk Apr 10, 2024

ppekrol Apr 9, 2024

maciejaszyk Apr 10, 2024

arekpalinski Apr 10, 2024

ppekrol Apr 10, 2024

	public enum NotificationType
	{
	None,
	AlertRaised,
	OperationChanged,
	DatabaseChanged,
	NotificationUpdated,
	RecentError, // used in studio
	PerformanceHint,
	DatabaseStatsChanged,
	ClusterTopologyChanged

RavenDB-22176 Exposing notifications via GET endpoint #18318

RavenDB-22176 Exposing notifications via GET endpoint #18318

Conversation

maciejaszyk commented Apr 2, 2024

Issue link

Additional description

Type of change

How risky is the change?

Backward compatibility

Is it platform specific issue?

Documentation update

Testing by Contributor

Testing by RavenDB QA team

Is there any existing behavior change of other features due to this change?

UI work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment