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.

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

Local stored response fetching #3600

Merged
merged 5 commits into from
Apr 10, 2024
Merged

Conversation

zhongshixi
Copy link
Contributor

@zhongshixi zhongshixi commented Mar 28, 2024

Problem

  1. make the local stored response fetcher a separate and independent PR according to the feed back from Hello World Sample  #3326 (comment)
We also think fetcher change should be a separate PR with unit tests, not a part of this PR. I also understand stored responses set up will not work without File System fetcher changes. Maybe fetcher PR should go first.

this PR is to unblock the hello world example framework i try to build

  1. add unit test coverage for the stored response

@zhongshixi zhongshixi changed the title Local stored response Local stored response Fetching Mar 28, 2024
@zhongshixi zhongshixi changed the title Local stored response Fetching Local stored response fetching Mar 28, 2024
@zhongshixi
Copy link
Contributor Author

cc @bretg for visibility
cc @VeronikaSolovei9 @guscarreon for code review

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, it works locally, I can execute requests with stored responses successfully. Test looks good too.

func (fetcher *eagerFetcher) FetchResponses(ctx context.Context, ids []string) (data map[string]json.RawMessage, errs []error) {
return nil, nil
storedResponses := fetcher.FileSystem.Directories["stored_responses"].Files
Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 Mar 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per Gus suggestion, please use "data", not "storedResponses" #3326 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VeronikaSolovei9 @guscarreon
have updated the code implementation

@zhongshixi
Copy link
Contributor Author

cc @SyntaxNode for visibility & code review ( sorry i forgot to mention you)

Copy link
Contributor

@AlexBVolcy AlexBVolcy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments left

@@ -35,8 +36,15 @@ func (fetcher *eagerFetcher) FetchRequests(ctx context.Context, requestIDs []str
return storedRequests, storedImpressions, errs
}

// Fetch Responses - Implements the interface to read the stored response information from the fetcher's FileSystem, the directory name is "stored_responses"
func (fetcher *eagerFetcher) FetchResponses(ctx context.Context, ids []string) (data map[string]json.RawMessage, errs []error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: what do you think about renaming fetcher here to just be f

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexBVolcy i think that would be a different discussion, all the other fetchers, e.g http_fetcher, empty_fetcher and db_fetcherare all using the samefetcher` as the receiver name

@@ -25,6 +27,55 @@ func TestFileFetcher(t *testing.T) {
validateImp(t, storedImps)
}

func TestStoredResponseFileFetcher(t *testing.T) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: Can you delete this space

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleted

Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should FetchResponses throw an error when the parameter ids is empty? If so, can we add the check and its corresponding test case in TestStoredResponseFileFetcher?

@zhongshixi
Copy link
Contributor Author

zhongshixi commented Apr 5, 2024

Should FetchResponses throw an error when the parameter ids is empty? If so, can we add the check and its corresponding test case in TestStoredResponseFileFetcher?

@guscarreon
i do not think so

  1. As it changes the behaviour expectation of the interface. I want to make it consistent with what FetchRequests behaves - if you pass empty id, you get empty json.

  2. This is also consistent with db_fetcher's behaviour as well, if we want to change the behaviour, it should be different PR and different discussion in my opinion

Screenshot 2024-04-04 at 10 20 14 PM
  1. you actually do not need to change the behaviour, the consumer of the function will ensure the method will not be invoked if id list is empty
Screenshot 2024-04-04 at 10 30 20 PM

@bsardo bsardo assigned VeronikaSolovei9 and unassigned guscarreon Apr 8, 2024
@zhongshixi
Copy link
Contributor Author

thanks @VeronikaSolovei9 for approval
need one more approval to get merged :)

Copy link
Contributor

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bsardo bsardo merged commit 9cd1b3d into prebid:master Apr 10, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants