-
Notifications
You must be signed in to change notification settings - Fork 820
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
RavenDB-21679 Require to assign AuthorizationStatus #18506
Conversation
AuthenticateConnection = HttpContext.Features.Get<IHttpAuthenticationFeature>() as AuthenticateConnection; | ||
} | ||
|
||
public AuthorizationStatus GetAuthorizationStatus(string databaseName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this and use it directly via RequestHandler.GetXYZ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
src/Raven.Server/Smuggler/Documents/Data/DatabaseSmugglerOptionsServerSide.cs
Show resolved
Hide resolved
src/Raven.Server/Smuggler/Documents/Handlers/SmugglerHandler.cs
Outdated
Show resolved
Hide resolved
src/Raven.Server/Smuggler/Documents/Data/DatabaseSmugglerOptionsServerSide.cs
Outdated
Show resolved
Hide resolved
...n.Server/Documents/Handlers/Processors/Smuggler/AbstractSmugglerHandlerProcessorForImport.cs
Outdated
Show resolved
Hide resolved
21965dd
to
c2ab941
Compare
...er/Documents/Handlers/Admin/Processors/Indexes/AdminIndexHandlerProcessorForJavaScriptPut.cs
Outdated
Show resolved
Hide resolved
src/Raven.Server/Documents/Handlers/LegacyReplicationHandler.cs
Outdated
Show resolved
Hide resolved
...rver/Documents/Handlers/Processors/SampleData/SampleDataHandlerProcessorForPostSampleData.cs
Outdated
Show resolved
Hide resolved
protected AbstractDatabaseHandlerProcessor([NotNull] TRequestHandler requestHandler) : base(requestHandler) | ||
{ | ||
ContextPool = requestHandler.ContextPool; | ||
Logger = requestHandler.Logger; | ||
AuthenticateConnection = HttpContext.Features.Get<IHttpAuthenticationFeature>() as AuthenticateConnection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, this is not free (this is an IEnumerable) and you are now performing that for every single call to the database, even if this is not needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
6b6bdb9
to
bc2e447
Compare
Please check failing tests - https://jenkins.hibernatingrhinos.com/job/PR_Tests/10867/ |
Issue link
https://issues.hibernatingrhinos.com/issue/RavenDB-21679
Additional description
We encountered a situation where the AuthorizationStatus was not assigned in the DatabaseSmugglerOptionsServerSide class.
Now, we ensure that it's assigned when creating a new instance.
Type of change
How risky is the change?
Backward compatibility
Is it platform specific issue?
Documentation update
Documentation Required
tag.Testing by Contributor
private
)Testing by RavenDB QA team
QA Required
tag.Is there any existing behavior change of other features due to this change?
UI work
Studio Required
tag.