(BEDS-848) Move search and cursor parameter validation to API #1014

remoterami · 2024-10-22T12:24:12Z

No description provided.

LuccaBitfly · 2024-10-23T07:48:14Z

backend/pkg/api/enums/validator_dashboard_enums.go

+func (s VDBBlocksSearches) GetSearches() []SearchType {
+	return []SearchType{
+		Integer,
+		ValidatorPublicKeyWithPrefix,
+		Name,
+	}
+}


As discussed, we don't really need to provide a list of search types if we always check against every search type and just ignore them in SetSearchType

It's optional now

LuccaBitfly · 2024-10-23T07:49:39Z

backend/pkg/api/enums/validator_dashboard_enums.go

@@ -170,6 +170,35 @@ var VDBBlocksColumns = struct {
 	VDBBlockProposerReward,
 }

+type VDBBlocksSearches struct {


Don't know if these structs fit in the enum package. But also don't know where else they'd fit.

I moved it to types, I think that's slightly better, ptal

LuccaBitfly · 2024-10-23T07:50:27Z

backend/pkg/api/handlers/input_validation.go

@@ -416,6 +435,17 @@ func checkSort[T enums.EnumFactory[T]](v *validationError, sortString string) *t
 	return &types.Sort[T]{Column: sortCol, Desc: desc}
 }

+func checkSearch[T enums.Searchable](searchString string) (T, error) {


This func should prob never return an error, in worst case the returning searchable should be empty

It's returning a bool now to indicate an invalid search (if false), aka the search query didn't match the rules for this search. This includes accepted pattern matching but it may also check extra conditions. E.g. if the aggregateGroups flag is set, the group search is not applied even if the regex would otherwise match. See the three current examples, e.g. https://github.com/gobitfly/beaconchain/pull/1014/files#diff-eeb9062bd8258c1d32e584699f9dc786ad81df60981ec50fbc17b3c8962d7581R666-R669

Motivation for this approach was to allow early-outing with empty responses on the api layer already if possible, before even invoking data access at all.

LuccaBitfly · 2024-10-23T08:04:01Z

backend/pkg/api/enums/enums.go

+	Name SearchType = iota // default
+	Integer
+	EthereumAddress
+	WithdrawalCredential
+	EnsName
+	NonEmpty
+	Graffiti
+	Cursor
+	Email
+	Password
+	EmailUserToken
+	JsonContentType
+	// Validator Dashboard
+	ValidatorDashboardPublicId
+	ValidatorPublicKeyWithPrefix
+	ValidatorPublicKey


Since const namespace is package-scoped, pls prefix these enums with SearchType..., like with the other enums.

cloudflare-workers-and-pages · 2024-10-23T10:13:00Z

Deploying beaconchain with Cloudflare Pages

Latest commit:	`2acc450`
Status:	✅ Deploy successful!
Preview URL:	https://bc262cc5.beaconchain.pages.dev
Branch Preview URL:	https://beds-848-move-search-cursor.beaconchain.pages.dev

View logs

LuccaBitfly · 2024-11-22T14:57:36Z

backend/pkg/api/handlers/handler_service.go

@@ -267,6 +267,10 @@ func writeResponse(w http.ResponseWriter, r *http.Request, statusCode int, respo
 	}
 }

+func emptyPagingResponse() interface{} {


Suggested change

func emptyPagingResponse() interface{} {

func emptyPagingResponse() types.ApiPagingResponse[any] {

LuccaBitfly · 2024-11-22T14:59:39Z

backend/pkg/api/types/data_access.go

I think it would be better if we seperate the types here. Cursors should probably go in their own cursor.go, and searches should also probably get their own. We prob have to find some good naming to differentiate between the search endpoint and the search filters on tables.

LuccaBitfly · 2024-11-22T15:05:24Z

backend/pkg/api/handlers/public.go

-	data, paging, err := h.getDataAccessor(r).GetValidatorDashboardValidators(r.Context(), *dashboardId, groupId, pagingParams.cursor, *sort, pagingParams.search, pagingParams.limit)
+	search := &types.VDBManageValidatorsSearch{DashboardId: *dashboardId}
+	if !checkSearch(search, pagingParams.search) {
+		returnOk(w, r, emptyPagingResponse())


needs a return statement after this (also for all other checkSearch calls)

should maybe also discuss with team whether we want to early return, ignore, or return error (if you did and this was the conclusion please lmk)

LuccaBitfly · 2024-11-22T15:22:28Z

backend/pkg/api/types/data_access.go

+type SearchNumber baseSearchResult[uint64]
+type SearchString baseSearchResult[string]
+
+func (bs *basicSearch) AsNumber(st SearchType) SearchNumber {


Don't get why this needs to have a SearchType argument, feels like it unnecessarily complicates the API of using searchtypes.

It was meant to enable increased but unified flexibility in case that different types should be returned in different formats, e.g. to scale ETH -> wei values or something. Atm I also can't think of many examples (removing/adding an optional 0x prefix could be one though), so it's true that this looks like unneeded overhead, didn't think it's a big issue as this is encapsulated away from every-day data access workflow. But I'll disable it for now until a real usecase comes up

LuccaBitfly · 2024-11-22T15:22:55Z

backend/pkg/api/types/data_access.go

+	case SearchTypeInteger:
+		number, err := strconv.ParseUint(bs.value, 10, 64)
+		if err != nil {
+			log.Error(err, "error converting search value, check regex parsing", 0)


this should definitely return the error and not just log it

I tried to explicitly design the entire validation process in a way that api handles errors early so that data access doesn't have to deal with catching anymore at the point of using the result, as this is kind of the point of everything and greatly improves readability. Converting the raw string result to the expected format should happen earlier though, I'll try to revert back to using struct fields instead of methods

LuccaBitfly · 2024-11-22T15:26:16Z

backend/pkg/api/types/data_access.go

+	return SearchNumber{}
+}
+
+func (bs *basicSearch) AsString(st SearchType) SearchString {


Again, I think the SearchTypes arg makes this more complicated than necessary. I think in practice the data access func would rather just want to have a .GetValue() and format the value itself rather than have something funky happening hidden behind a AsString(). If it turns out we want to reuse formatting logic, it should happen in another func IMO.

Okay I've removed these methods and gone a step in that direction, ptal. Wouldn't remove the conversion entirely as doing it separately requires error handling in data access which would be a form of input validation.

LuccaBitfly · 2024-11-22T15:27:48Z

backend/pkg/api/handlers/public.go


-	data, paging, err := h.getDataAccessor(r).GetValidatorDashboardSummary(r.Context(), *dashboardId, period, pagingParams.cursor, *sort, pagingParams.search, pagingParams.limit, protocolModes)
+	data, paging, err := h.getDataAccessor(r).GetValidatorDashboardSummary(r.Context(), *dashboardId, period, "", *sort, *search, pagingParams.limit, protocolModes)


Is passing the "" intentional?

Yes basically, atleast until we decided what to do here: https://github.com/gobitfly/beaconchain/pull/1014/files#diff-bc69fc9b7d7d8ba0514ef4cf9cb7f09971cb7860fd7460d0dc6e3ebeb76e7b1bR31

LuccaBitfly · 2024-11-22T15:29:19Z

backend/pkg/api/enums/enums.go

+import (
+	"time"
+)



minor: could remove this change

LuccaBitfly · 2024-11-22T15:36:36Z

backend/pkg/api/types/data_access.go

+	SearchTypePassword
+	SearchTypeEmailUserToken
+	SearchTypeJsonContentType


Why are these valid search types?

They are not, just copied all regexes for now

LuccaBitfly · 2024-11-22T15:38:59Z

backend/pkg/api/data_access/vdb.go

For all changes within data access you'd probably want to get the direct opinion of data access team members. For now in total this PR adds more lines than it removes, so currently it's achieving the same thing with more lines. BUT it's good and necessary, that the cursor validation happens in handlers, not in data access. I don't know whether you've applied these search type changes everywhere or if it will scale better in future (e.g. with account dashboard). So I can't form a full opinion here.

Yeah you're right, I'll also ask other members. Types haven't been applied everywhere yet, will do so next; imo it simplifies things but the changes also need thorough testing

LuccaBitfly requested changes Oct 23, 2024

View reviewed changes

remoterami force-pushed the BEDS-848/move-search-cursor-validation branch from cd7032b to 6a12467 Compare November 1, 2024 12:55

LuccaBitfly requested changes Nov 22, 2024

View reviewed changes

remoterami added 9 commits November 29, 2024 11:01

added generic search validation mechanism

e0c675a

runtime errors fixed, GetSearches optional

33fbb8b

renamed searches

9223f20

fix linter issue

d6d9f96

added custom return type conversion

9065a66

moved types to data access file

1553554

empty paging response method

7def72e

fixed runtime errors

8721e31

moved cursor validation to api

3a15cf4

remoterami force-pushed the BEDS-848/move-search-cursor-validation branch from 6a12467 to 3a15cf4 Compare November 29, 2024 10:11

resolved rebase conflicts

2acc450

remoterami force-pushed the BEDS-848/move-search-cursor-validation branch from 8e79e8f to 2acc450 Compare November 29, 2024 10:38

using struct fields instead of methods

af1c354

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(BEDS-848) Move search and cursor parameter validation to API #1014

(BEDS-848) Move search and cursor parameter validation to API #1014

remoterami commented Oct 22, 2024

LuccaBitfly Oct 23, 2024

remoterami Oct 23, 2024

LuccaBitfly Oct 23, 2024

remoterami Oct 28, 2024

LuccaBitfly Oct 23, 2024

remoterami Oct 28, 2024 •

edited

Loading

LuccaBitfly Oct 23, 2024

cloudflare-workers-and-pages bot commented Oct 23, 2024 •

edited

Loading

LuccaBitfly Nov 22, 2024

LuccaBitfly Nov 22, 2024

LuccaBitfly Nov 22, 2024

LuccaBitfly Nov 22, 2024

LuccaBitfly Nov 22, 2024

remoterami Nov 29, 2024 •

edited

Loading

LuccaBitfly Nov 22, 2024

remoterami Nov 29, 2024

LuccaBitfly Nov 22, 2024

remoterami Dec 9, 2024

LuccaBitfly Nov 22, 2024

remoterami Dec 9, 2024

LuccaBitfly Nov 22, 2024

LuccaBitfly Nov 22, 2024

remoterami Dec 9, 2024

LuccaBitfly Nov 22, 2024

remoterami Dec 9, 2024

	func emptyPagingResponse() interface{} {
	func emptyPagingResponse() types.ApiPagingResponse[any] {


		data, paging, err := h.getDataAccessor(r).GetValidatorDashboardSummary(r.Context(), dashboardId, period, pagingParams.cursor, sort, pagingParams.search, pagingParams.limit, protocolModes)
		data, paging, err := h.getDataAccessor(r).GetValidatorDashboardSummary(r.Context(), dashboardId, period, "", sort, *search, pagingParams.limit, protocolModes)

(BEDS-848) Move search and cursor parameter validation to API #1014

Are you sure you want to change the base?

(BEDS-848) Move search and cursor parameter validation to API #1014

Conversation

remoterami commented Oct 22, 2024

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

remoterami Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Oct 23, 2024 • edited Loading

Deploying beaconchain with Cloudflare Pages

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

remoterami Nov 29, 2024 • edited Loading

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

remoterami Oct 28, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 23, 2024 •

edited

Loading

remoterami Nov 29, 2024 •

edited

Loading