Skip to content

Commit

Permalink
PMM-12913 remove ID normalizers
Browse files Browse the repository at this point in the history
  • Loading branch information
ademidoff committed Jun 6, 2024
1 parent adbc985 commit 04b6196
Show file tree
Hide file tree
Showing 18 changed files with 57 additions and 142 deletions.
67 changes: 0 additions & 67 deletions managed/models/agent_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,6 @@ import (
"github.com/percona/pmm/version"
)

const (
agentIDPrefix = "/agent_id/"
serviceIDPrefix = "/service_id/"
nodeIDPrefix = "/node_id/"
actionIDPrefix = "/action_id/"
scheduledTaskIDPrefix = "/scheduled_task_id/"
artifactIDPrefix = "/artifact_id/"
restoreIDPrefix = "/restore_id/"
locationIDPrefix = "/location_id/"
)

// MySQLOptionsParams contains methods to create MySQLOptions object.
type MySQLOptionsParams interface {
GetTlsCa() string
Expand Down Expand Up @@ -166,62 +155,6 @@ func AzureOptionsFromRequest(params AzureOptionsParams) *AzureOptions {
return nil
}

// Add a prefix since gRPC does not allow to pass an URL path segment that begins with a slash.
// TODO: remove these Normalize functions once we drop prefixes in agent/service/node IDs.

// NormalizeAgentID adds a prefix to the agent ID if it does not already contain it.
func NormalizeAgentID(agentID string) string {
if agentID == "pmm-server" {
return agentID
}

return normalizeID(agentID, agentIDPrefix)
}

// NormalizeServiceID adds a prefix to the service ID if it does not already contain it.
func NormalizeServiceID(serviceID string) string {
return normalizeID(serviceID, serviceIDPrefix)
}

// NormalizeNodeID adds a prefix to the node ID if it does not already contain it.
func NormalizeNodeID(nodeID string) string {
return normalizeID(nodeID, nodeIDPrefix)
}

// NormalizeActionID adds a prefix to the node ID if it does not already contain it.
func NormalizeActionID(actionID string) string {
return normalizeID(actionID, nodeIDPrefix)
}

// NormalizeScheduledTaskID adds a prefix to the sheduled task ID if it does not already contain it.
func NormalizeScheduledTaskID(sTaskID string) string {
return normalizeID(sTaskID, scheduledTaskIDPrefix)
}

// NormalizeArtifactID adds a prefix to the artifact ID if it does not already contain it.
func NormalizeArtifactID(artifactID string) string {
return normalizeID(artifactID, artifactIDPrefix)
}

// NormalizeRestoreID adds a prefix to the restore ID if it does not already contain it.
func NormalizeRestoreID(restoreID string) string {
return normalizeID(restoreID, restoreIDPrefix)
}

// NormalizeLocationID adds a prefix to the location ID if it does not already contain it.
func NormalizeLocationID(locationID string) string {
return normalizeID(locationID, locationIDPrefix)
}

// normalizeID adds a prefix to ID if it does not already contain it.
func normalizeID(id, prefix string) string {
if id == "" || strings.HasPrefix(id, prefix) {
return id
}

return prefix + id
}

func checkUniqueAgentID(q *reform.Querier, id string) error {
if id == "" {
panic("empty Agent ID")
Expand Down
18 changes: 7 additions & 11 deletions managed/services/inventory/grpc/agents_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,9 @@ func agentType(req *inventoryv1.ListAgentsRequest) *models.AgentType {
// ListAgents returns a list of Agents for a given filters.
func (s *agentsServer) ListAgents(ctx context.Context, req *inventoryv1.ListAgentsRequest) (*inventoryv1.ListAgentsResponse, error) {
filters := models.AgentFilters{
PMMAgentID: models.NormalizeAgentID(req.GetPmmAgentId()),
NodeID: models.NormalizeNodeID(req.GetNodeId()),
ServiceID: models.NormalizeServiceID(req.GetServiceId()),
PMMAgentID: req.GetPmmAgentId(),
NodeID: req.GetNodeId(),
ServiceID: req.GetServiceId(),
AgentType: agentType(req),
}
agents, err := s.s.List(ctx, filters)
Expand Down Expand Up @@ -117,9 +117,7 @@ func (s *agentsServer) ListAgents(ctx context.Context, req *inventoryv1.ListAgen

// GetAgent returns a single Agent by ID.
func (s *agentsServer) GetAgent(ctx context.Context, req *inventoryv1.GetAgentRequest) (*inventoryv1.GetAgentResponse, error) {
agentID := models.NormalizeAgentID(req.GetAgentId())

agent, err := s.s.Get(ctx, agentID)
agent, err := s.s.Get(ctx, req.GetAgentId())
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -164,8 +162,7 @@ func (s *agentsServer) GetAgent(ctx context.Context, req *inventoryv1.GetAgentRe

// GetAgentLogs returns Agent logs by ID.
func (s *agentsServer) GetAgentLogs(ctx context.Context, req *inventoryv1.GetAgentLogsRequest) (*inventoryv1.GetAgentLogsResponse, error) {
agentID := models.NormalizeAgentID(req.GetAgentId())
logs, agentConfigLogLinesCount, err := s.s.Logs(ctx, agentID, req.GetLimit())
logs, agentConfigLogLinesCount, err := s.s.Logs(ctx, req.GetAgentId(), req.GetLimit())
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -214,7 +211,7 @@ func (s *agentsServer) AddAgent(ctx context.Context, req *inventoryv1.AddAgentRe

// ChangeAgent allows to change some Agent attributes.
func (s *agentsServer) ChangeAgent(ctx context.Context, req *inventoryv1.ChangeAgentRequest) (*inventoryv1.ChangeAgentResponse, error) {
agentID := models.NormalizeAgentID(req.GetAgentId())
agentID := req.GetAgentId()

switch req.Agent.(type) {
case *inventoryv1.ChangeAgentRequest_NodeExporter:
Expand Down Expand Up @@ -250,8 +247,7 @@ func (s *agentsServer) ChangeAgent(ctx context.Context, req *inventoryv1.ChangeA

// RemoveAgent removes the Agent.
func (s *agentsServer) RemoveAgent(ctx context.Context, req *inventoryv1.RemoveAgentRequest) (*inventoryv1.RemoveAgentResponse, error) {
agentID := models.NormalizeAgentID(req.GetAgentId())
if err := s.s.Remove(ctx, agentID, req.Force); err != nil {
if err := s.s.Remove(ctx, req.GetAgentId(), req.GetForce()); err != nil {
return nil, err
}

Expand Down
11 changes: 4 additions & 7 deletions managed/services/inventory/grpc/services_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,8 @@ func serviceType(serviceType inventoryv1.ServiceType) *models.ServiceType {

// ListServices returns a list of Services for a given filters.
func (s *servicesServer) ListServices(ctx context.Context, req *inventoryv1.ListServicesRequest) (*inventoryv1.ListServicesResponse, error) {
nodeID := models.NormalizeNodeID(req.GetNodeId())
filters := models.ServiceFilters{
NodeID: nodeID,
NodeID: req.GetNodeId(),
ServiceType: serviceType(req.GetServiceType()),
ExternalGroup: req.GetExternalGroup(),
}
Expand Down Expand Up @@ -114,8 +113,7 @@ func (s *servicesServer) ListActiveServiceTypes(

// GetService returns a single Service by ID.
func (s *servicesServer) GetService(ctx context.Context, req *inventoryv1.GetServiceRequest) (*inventoryv1.GetServiceResponse, error) {
serviceID := models.NormalizeServiceID(req.GetServiceId())
service, err := s.s.Get(ctx, serviceID)
service, err := s.s.Get(ctx, req.GetServiceId())
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -302,8 +300,7 @@ func (s *servicesServer) addExternalService(ctx context.Context, params *invento

// RemoveService removes Service.
func (s *servicesServer) RemoveService(ctx context.Context, req *inventoryv1.RemoveServiceRequest) (*inventoryv1.RemoveServiceResponse, error) {
serviceID := models.NormalizeServiceID(req.GetServiceId())
if err := s.s.Remove(ctx, serviceID, req.Force); err != nil {
if err := s.s.Remove(ctx, req.GetServiceId(), req.GetForce()); err != nil {
return nil, err
}

Expand All @@ -313,7 +310,7 @@ func (s *servicesServer) RemoveService(ctx context.Context, req *inventoryv1.Rem
// ChangeService changes service configuration.
func (s *servicesServer) ChangeService(ctx context.Context, req *inventoryv1.ChangeServiceRequest) (*inventoryv1.ChangeServiceResponse, error) {
sl := &models.ChangeStandardLabelsParams{
ServiceID: models.NormalizeServiceID(req.GetServiceId()),
ServiceID: req.ServiceId,
Cluster: req.Cluster,
Environment: req.Environment,
ReplicationSet: req.ReplicationSet,
Expand Down
3 changes: 1 addition & 2 deletions managed/services/inventory/nodes.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@ func (s *NodesService) Get(ctx context.Context, req *inventoryv1.GetNodeRequest)
modelNode := &models.Node{}
e := s.db.InTransactionContext(ctx, nil, func(tx *reform.TX) error {
var err error
nodeID := models.NormalizeNodeID(req.NodeId)
modelNode, err = models.FindNodeByID(tx.Querier, nodeID)
modelNode, err = models.FindNodeByID(tx.Querier, req.NodeId)
if err != nil {
return err
}
Expand Down
10 changes: 4 additions & 6 deletions managed/services/management/backup/artifacts_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,7 @@ func (s *ArtifactsService) ListArtifacts(context.Context, *backuppb.ListArtifact

// DeleteArtifact deletes specified artifact and its files.
func (s *ArtifactsService) DeleteArtifact(ctx context.Context, req *backuppb.DeleteArtifactRequest) (*backuppb.DeleteArtifactResponse, error) { //nolint:revive
artifactID := models.NormalizeArtifactID(req.ArtifactId)
artifact, err := models.FindArtifactByID(s.db.Querier, artifactID)
artifact, err := models.FindArtifactByID(s.db.Querier, req.ArtifactId)
if err != nil {
return nil, err
}
Expand All @@ -120,7 +119,7 @@ func (s *ArtifactsService) DeleteArtifact(ctx context.Context, req *backuppb.Del

storage := backup.GetStorageForLocation(location)

if err := s.removalSVC.DeleteArtifact(storage, artifactID, req.RemoveFiles); err != nil {
if err := s.removalSVC.DeleteArtifact(storage, req.ArtifactId, req.RemoveFiles); err != nil {
return nil, err
}
return &backuppb.DeleteArtifactResponse{}, nil
Expand All @@ -131,11 +130,10 @@ func (s *ArtifactsService) ListPitrTimeranges(ctx context.Context, req *backuppb
var artifact *models.Artifact
var err error

artifactID := models.NormalizeArtifactID(req.ArtifactId)
artifact, err = models.FindArtifactByID(s.db.Querier, artifactID)
artifact, err = models.FindArtifactByID(s.db.Querier, req.ArtifactId)
if err != nil {
if errors.Is(err, models.ErrNotFound) {
return nil, status.Errorf(codes.NotFound, "Artifact with ID '%s' not found.", artifactID)
return nil, status.Errorf(codes.NotFound, "Artifact with ID '%s' not found.", req.ArtifactId)
}
return nil, err
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ func TestListPitrTimeranges(t *testing.T) {
response, err := artifactsService.ListPitrTimeranges(ctx, &backuppb.ListPitrTimerangesRequest{
ArtifactId: unknownID,
})
unknownID = models.NormalizeArtifactID(unknownID)
tests.AssertGRPCError(t, status.New(codes.NotFound, fmt.Sprintf("Artifact with ID '%s' not found.", unknownID)), err)
assert.Nil(t, response)
})
Expand Down
12 changes: 5 additions & 7 deletions managed/services/management/backup/backups_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -393,8 +393,7 @@ func (s *BackupService) ChangeScheduledBackup(ctx context.Context, req *backuppb

// RemoveScheduledBackup stops and removes existing scheduled backup task.
func (s *BackupService) RemoveScheduledBackup(ctx context.Context, req *backuppb.RemoveScheduledBackupRequest) (*backuppb.RemoveScheduledBackupResponse, error) {
scheduledBackupID := models.NormalizeScheduledTaskID(req.ScheduledBackupId)
task, err := models.FindScheduledTaskByID(s.db.Querier, scheduledBackupID)
task, err := models.FindScheduledTaskByID(s.db.Querier, req.ScheduledBackupId)
if err != nil {
return nil, err
}
Expand All @@ -412,7 +411,7 @@ func (s *BackupService) RemoveScheduledBackup(ctx context.Context, req *backuppb

errTx := s.db.InTransactionContext(ctx, nil, func(tx *reform.TX) error {
artifacts, err := models.FindArtifacts(tx.Querier, models.ArtifactFilters{
ScheduleID: scheduledBackupID,
ScheduleID: req.ScheduledBackupId,
})
if err != nil {
return err
Expand All @@ -427,7 +426,7 @@ func (s *BackupService) RemoveScheduledBackup(ctx context.Context, req *backuppb
}
}

return s.scheduleService.Remove(scheduledBackupID)
return s.scheduleService.Remove(req.ScheduledBackupId)
})
if errTx != nil {
return nil, errTx
Expand All @@ -452,7 +451,7 @@ func (s *BackupService) GetLogs(_ context.Context, req *backuppb.GetLogsRequest)
},
}

jobsFilter.ArtifactID = models.NormalizeArtifactID(req.ArtifactId)
jobsFilter.ArtifactID = req.ArtifactId

jobs, err := models.FindJobs(s.db.Querier, jobsFilter)
if err != nil {
Expand Down Expand Up @@ -500,8 +499,7 @@ func (s *BackupService) ListArtifactCompatibleServices(
ctx context.Context,
req *backuppb.ListArtifactCompatibleServicesRequest,
) (*backuppb.ListArtifactCompatibleServicesResponse, error) {
artifactID := models.NormalizeActionID(req.ArtifactId)
compatibleServices, err := s.compatibilityService.FindArtifactCompatibleServices(ctx, artifactID)
compatibleServices, err := s.compatibilityService.FindArtifactCompatibleServices(ctx, req.ArtifactId)
switch {
case err == nil:
case errors.Is(err, models.ErrNotFound):
Expand Down
2 changes: 1 addition & 1 deletion managed/services/management/backup/backups_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,7 @@ func TestGetLogs(t *testing.T) {
}

t.Run("get backup logs", func(t *testing.T) {
artifactID := models.NormalizeArtifactID(uuid.New().String())
artifactID := uuid.New().String()
job, err := models.CreateJob(db.Querier, models.CreateJobParams{
PMMAgentID: "agent",
Type: models.MongoDBBackupJob,
Expand Down
6 changes: 2 additions & 4 deletions managed/services/management/backup/locations_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,7 @@ func (s *LocationsService) ChangeLocation(ctx context.Context, req *backuppb.Cha
}

err := s.db.InTransaction(func(tx *reform.TX) error {
locationID := models.NormalizeLocationID(req.LocationId)
_, err := models.ChangeBackupLocation(tx.Querier, locationID, params)
_, err := models.ChangeBackupLocation(tx.Querier, req.LocationId, params)
if err != nil {
return err
}
Expand Down Expand Up @@ -230,8 +229,7 @@ func (s *LocationsService) RemoveLocation(ctx context.Context, req *backuppb.Rem
}

err := s.db.InTransactionContext(ctx, nil, func(tx *reform.TX) error {
locationID := models.NormalizeLocationID(req.LocationId)
return models.RemoveBackupLocation(tx.Querier, locationID, mode)
return models.RemoveBackupLocation(tx.Querier, req.LocationId, mode)
})
if err != nil {
return nil, err
Expand Down
2 changes: 1 addition & 1 deletion managed/services/management/backup/restore_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ func (s *RestoreService) GetLogs(_ context.Context, req *backupv1.RestoreService
},
}

jobsFilter.RestoreID = models.NormalizeRestoreID(req.RestoreId)
jobsFilter.RestoreID = req.RestoreId

jobs, err := models.FindJobs(s.db.Querier, jobsFilter)
if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions managed/services/management/backup/restore_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ func TestRestoreServiceGetLogs(t *testing.T) {
}

t.Run("get physical restore logs", func(t *testing.T) {
restoreID := models.NormalizeRestoreID(uuid.New().String())
restoreID := uuid.New().String()
job, err := models.CreateJob(db.Querier, models.CreateJobParams{
PMMAgentID: "agent",
Type: models.MongoDBBackupJob,
Expand Down Expand Up @@ -107,7 +107,7 @@ func TestRestoreServiceGetLogs(t *testing.T) {
})

t.Run("get logical restore logs", func(t *testing.T) {
restoreID := models.NormalizeRestoreID(uuid.New().String())
restoreID := uuid.New().String()
logicalRestore, err := models.CreateJob(db.Querier, models.CreateJobParams{
PMMAgentID: "agent",
Type: models.MongoDBBackupJob,
Expand Down
5 changes: 2 additions & 3 deletions managed/services/management/checks.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (

advisorsv1 "github.com/percona/pmm/api/advisors/v1"
managementv1 "github.com/percona/pmm/api/management/v1"
"github.com/percona/pmm/managed/models"
"github.com/percona/pmm/managed/services"
)

Expand Down Expand Up @@ -114,8 +113,8 @@ func (s *ChecksAPIService) ListFailedServices(ctx context.Context, _ *advisorsv1

// GetFailedChecks returns details of failed checks for a given service.
func (s *ChecksAPIService) GetFailedChecks(ctx context.Context, req *advisorsv1.GetFailedChecksRequest) (*advisorsv1.GetFailedChecksResponse, error) {
serviceID := models.NormalizeServiceID(req.ServiceId)
results, err := s.checksService.GetChecksResults(ctx, serviceID)

results, err := s.checksService.GetChecksResults(ctx, req.ServiceId)
if err != nil {
if errors.Is(err, services.ErrAdvisorsDisabled) {
return nil, status.Errorf(codes.FailedPrecondition, "%v.", err)
Expand Down
3 changes: 1 addition & 2 deletions managed/services/management/grpc/actions_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,7 @@ func NewActionsServer(a *agents.ActionsService, db *reform.DB) actionsv1.Actions

// GetAction gets an action result.
func (s *actionsServer) GetAction(ctx context.Context, req *actionsv1.GetActionRequest) (*actionsv1.GetActionResponse, error) { //nolint:revive
actionID := models.NormalizeActionID(req.ActionId)
res, err := models.FindActionResultByID(s.db.Querier, actionID)
res, err := models.FindActionResultByID(s.db.Querier, req.ActionId)
if err != nil {
return nil, err
}
Expand Down
5 changes: 2 additions & 3 deletions managed/services/management/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,13 @@ func (s *ManagementService) RegisterNode(ctx context.Context, req *managementv1.

// Unregister do unregistration of the node.
func (s *ManagementService) Unregister(ctx context.Context, req *managementv1.UnregisterNodeRequest) (*managementv1.UnregisterNodeResponse, error) {
nodeID := models.NormalizeNodeID(req.NodeId)
node, err := models.FindNodeByID(s.db.Querier, nodeID)
node, err := models.FindNodeByID(s.db.Querier, req.NodeId)
if err != nil {
return nil, err
}

err = s.db.InTransaction(func(tx *reform.TX) error {
return models.RemoveNode(tx.Querier, nodeID, models.RemoveCascade)
return models.RemoveNode(tx.Querier, req.NodeId, models.RemoveCascade)
})
if err != nil {
return nil, err
Expand Down
4 changes: 2 additions & 2 deletions managed/services/management/node_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -317,8 +317,8 @@ func TestNodeService(t *testing.T) {
}

const (
nodeExporterID = "/agent_id/00000000-0000-4000-8000-000000000001"
postgresqlServiceID = "/service_id/00000000-0000-4000-8000-000000000002"
nodeExporterID = "00000000-0000-4000-8000-000000000001"
postgresqlServiceID = "00000000-0000-4000-8000-000000000002"
)

t.Run("should output an unfiltered list of all nodes", func(t *testing.T) {
Expand Down
Loading

0 comments on commit 04b6196

Please sign in to comment.