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.

Sign up for GitHub

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

Jump to bottom

Remove CatchAll and body ID manipulation since it should no longer be needed #3987

Merged
merged 3 commits into from Mar 12, 2024
Merged

Remove CatchAll and body ID manipulation since it should no longer be needed #3987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d20d016
No longer replace the message id since the body will be loaded using …
danielmarbach Mar 5, 2024
00e5f99
Remove the catch all part because it was only handling /body which sh…
danielmarbach Mar 5, 2024
cd1edaf
Remove the replacement of forward slashes since that could never have…
danielmarbach Mar 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion src/ServiceControl.Audit.UnitTests/ApprovalFiles/APIApprovals.HttpApiRoutes.approved.txt
View file
Open in desktop
Expand Up @@ -9,7 +9,6 @@ GET /endpoints/{endpoint}/messages/search/{keyword} => ServiceControl.Audit.Audi
GET /endpoints/known => ServiceControl.Audit.Monitoring.KnownEndpointsController:GetAll(PagingInfo pagingInfo)
GET /instance-info => ServiceControl.Audit.Infrastructure.WebApi.RootController:Config()
GET /messages => ServiceControl.Audit.Auditing.MessagesView.GetMessagesController:GetAllMessages(PagingInfo pagingInfo, SortInfo sortInfo, Boolean includeSystemMessages)
GET /messages/{*catchAll} => ServiceControl.Audit.Auditing.MessagesView.GetMessagesController:CatchAll(String catchAll)
GET /messages/{id}/body => ServiceControl.Audit.Auditing.MessagesView.GetMessagesController:Get(String id)
GET /messages/search => ServiceControl.Audit.Auditing.MessagesView.GetMessagesController:Search(PagingInfo pagingInfo, SortInfo sortInfo, String q)
GET /messages/search/{keyword} => ServiceControl.Audit.Auditing.MessagesView.GetMessagesController:SearchByKeyWord(PagingInfo pagingInfo, SortInfo sortInfo, String keyword)
Expand Down
27 changes: 3 additions & 24 deletions src/ServiceControl.Audit/Auditing/MessagesView/GetMessagesController.cs
View file
Open in desktop
Expand Up @@ -8,8 +8,6 @@ namespace ServiceControl.Audit.Auditing.MessagesView
using Microsoft.AspNetCore.Mvc;
using Persistence;

// All routes matching `messages/*` must be in this controller as WebAPI cannot figure out the overlapping routes
// from `messages/{*catchAll}` if they're in separate controllers.
[ApiController]
[Route("api")]
public class GetMessagesController(IAuditDataStore dataStore) : ControllerBase
Expand Down Expand Up @@ -45,10 +43,7 @@ public async Task<IList<AuditCount>> GetEndpointAuditCounts([FromQuery] PagingIn
[HttpGet]
public async Task<IActionResult> Get(string id)
{
var messageId = id;
messageId = messageId?.Replace("/", @"\");

var result = await dataStore.GetMessageBody(messageId);
var result = await dataStore.GetMessageBody(id);

if (result.Found == false)
{
Expand All @@ -62,30 +57,14 @@ public async Task<IActionResult> Get(string id)

if (result.StringContent == null && result.StreamContent == null)
{
throw new Exception($"Metadata for message '{messageId}' indicated that a body was present but no content could be found in storage");
throw new Exception($"Metadata for message '{id}' indicated that a body was present but no content could be found in storage");
}

Response.Headers.ETag = result.ETag;
var contentType = result.ContentType ?? "text/*";
return result.StringContent != null ? Content(result.StringContent, contentType) : File(result.StreamContent, contentType);
}

// TODO: Verify if this catch all approach is still relevant today with Kestrel
// Possible a message may contain a slash or backslash, either way http.sys will rewrite it to forward slash,
// and then the "normal" route above will not activate, resulting in 404 if this route is not present.
[Route("messages/{*catchAll}")]
[HttpGet]
public async Task<IActionResult> CatchAll(string catchAll)
{
if (!string.IsNullOrEmpty(catchAll) && catchAll.EndsWith("/body"))
{
var id = catchAll.Substring(0, catchAll.Length - 5);
return await Get(id);
}

return NotFound();
}

[Route("messages/search")]
[HttpGet]
public async Task<IList<MessagesView>> Search([FromQuery] PagingInfo pagingInfo, [FromQuery] SortInfo sortInfo, string q)
Expand All @@ -99,7 +78,7 @@ public async Task<IList<MessagesView>> Search([FromQuery] PagingInfo pagingInfo,
[HttpGet]
public async Task<IList<MessagesView>> SearchByKeyWord([FromQuery] PagingInfo pagingInfo, [FromQuery] SortInfo sortInfo, string keyword)
{
var result = await dataStore.QueryMessages(keyword?.Replace("/", @"\"), pagingInfo, sortInfo);
var result = await dataStore.QueryMessages(keyword, pagingInfo, sortInfo);
Response.WithQueryStatsAndPagingInfo(result.QueryStats, pagingInfo);
return result.Results;
}
Expand Down
1 change: 0 additions & 1 deletion src/ServiceControl.UnitTests/ApprovalFiles/APIApprovals.HttpApiRoutes.approved.txt
View file
Open in desktop
Expand Up @@ -40,7 +40,6 @@ GET /heartbeats/stats => ServiceControl.Monitoring.EndpointsMonitoringController
GET /instance-info => ServiceControl.Infrastructure.WebApi.RootController:Config()
GET /license => ServiceControl.Licensing.LicenseController:License(Boolean refresh)
GET /messages => ServiceControl.CompositeViews.Messages.GetMessagesController:Messages(PagingInfo pagingInfo, SortInfo sortInfo, Boolean includeSystemMessages)
GET /messages/{*catchAll} => ServiceControl.CompositeViews.Messages.GetMessagesController:CatchAll(String catchAll, String instanceId)
GET /messages/{id}/body => ServiceControl.CompositeViews.Messages.GetMessagesController:Get(String id, String instanceId)
GET /messages/search => ServiceControl.CompositeViews.Messages.GetMessagesController:Search(PagingInfo pagingInfo, SortInfo sortInfo, String q)
GET /messages/search/{keyword} => ServiceControl.CompositeViews.Messages.GetMessagesController:SearchByKeyWord(PagingInfo pagingInfo, SortInfo sortInfo, String keyword)
Expand Down
16 changes: 0 additions & 16 deletions src/ServiceControl/CompositeViews/Messages/GetMessagesController.cs
View file
Open in desktop
Expand Up @@ -87,22 +87,6 @@ public async Task<IActionResult> Get(string id, [FromQuery(Name = "instance_id")
return Empty;
}

// TODO Is this still needed?
// Possible a message may contain a slash or backslash, either way http.sys will rewrite it to forward slash,
// and then the "normal" route above will not activate, resulting in 404 if this route is not present.
[Route("messages/{*catchAll}")]
[HttpGet]
public async Task<IActionResult> CatchAll(string catchAll, [FromQuery(Name = "instance_id")] string instanceId)
{
if (!string.IsNullOrEmpty(catchAll) && catchAll.EndsWith("/body"))
{
var id = catchAll[..^5];
return await Get(id, instanceId);
}

return NotFound();
}

[Route("messages/search")]
[HttpGet]
public Task<IList<MessagesView>> Search([FromQuery] PagingInfo pagingInfo, [FromQuery] SortInfo sortInfo, string q) => api.Execute(new(pagingInfo, sortInfo, q));
Expand Down