Details

Reviewers

varun
bartek

Commits

rCOMMf5ae0f430dc6: [identity] [6/n] Requires a Search Query json message from client for user…

Summary

This requires the client to send a Search Query json object when using identity search. This makes it clear the user search type and query (i.e. prefix vs exact).

As an example, after authenticating with the websocket server, to prefix search, you would send:

{"prefix": "..."}

Depends on D10851

Test Plan

tested locally after authenticating.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

will created this revision.Jan 29 2024, 11:50 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJan 29 2024, 11:50 AM

will retitled this revision from [services] Requires a Search Query json message from client for user identity search to [services] [6/n] Requires a Search Query json message from client for user identity search.Jan 29 2024, 11:51 AM

will edited the summary of this revision. (Show Details)

will added a parent revision: D10851: [identity] [5/n] Create search result message type for identity search.

will added inline comments.Jan 29 2024, 11:55 AM

shared/identity_search_messages/src/messages/search_query.rs
13 ↗	(On Diff #36292)	Exact would be another search type to be implemented

Harbormaster completed remote builds in B26264: Diff 36292.Jan 29 2024, 12:10 PM

will requested review of this revision.Jan 29 2024, 12:10 PM

Converting create_json_query to async handle function for prefix searches

Splits accept_connection into handlers of different search types

will added inline comments.Jan 29 2024, 1:25 PM

services/identity/src/websockets/mod.rs
143 ↗	(On Diff #36311)	Code originally in accept_connection is moved here. Planning on adding a `handle_exact_search function` later.

will added inline comments.Jan 29 2024, 1:28 PM

services/identity/src/websockets/mod.rs
182 ↗	(On Diff #36311)	Not sure if it's okay for a handler returns a String. Unsure of our convention and whether handle_prefix search should also handle sending messages back to client or I can just have its caller (`accept_connection`) send `search_result`

will marked an inline comment as not done.Jan 29 2024, 1:30 PM

Harbormaster completed remote builds in B26278: Diff 36311.Jan 29 2024, 1:41 PM

will retitled this revision from [services] [6/n] Requires a Search Query json message from client for user identity search to [identity] [6/n] Requires a Search Query json message from client for user identity search.Jan 30 2024, 11:07 AM

Business logic looks good to me.
Generally, there are lots of serde_json::from_str and serde_json::to_string, map_err everywhere which can be simplified and generalized using some traits and generics but it's not worth spending much time on this.

Some general tips to make the code clean and concise:

reqwest allows Serialize / Deserialize types to be passed directly as JSON request / response bodies, without explicit serde_json calls. See for instance this function in comm-lib
Implementing impl From<serde_json::Error> for errors::WebsocketError` etc. would allow you to use the ? syntax and avoid all these map_err

// somewhere in errors.rs
impl From<serde_json::Error> for WebsocketError {
  fn from(_err: serde_json::Error) -> Self {
    // optionally some tracing::error!() here
    WebsocketError::SerializationError
  }
}

impl From<reqwest::Error> for WebsocketError {
  fn from(_err: reqwest::Error) -> Self {
    // optionally some tracing::error!() here
    WebsocketError::SearchError
  }
}

services/identity/src/websockets/mod.rs
154–159 ↗	(On Diff #36311)	Nit, a bit more concise syntax. Or just use `map_err()` like you do below
164–174 ↗	(On Diff #36311)	Assuming `response` is a `reqwest::Response` (looking at `send_search_request` return type), this can be simplified (see my general comment). Or even more, assuming error traits impl: let search_response: SearchResponse<User> = send_search_request(&opensearch_url, json_body) .await? .json() .await?;
182 ↗	(On Diff #36311)	I think it's okay the way it is now
shared/identity_search_messages/src/messages/search_query.rs
11 ↗	(On Diff #36311)	Why is this untagged while others have `tag = type`? Isn't it better to keep it concise across all `Messages` types?

will added inline comments.Jan 31 2024, 1:05 PM

shared/identity_search_messages/src/messages/search_query.rs
11 ↗	(On Diff #36311)	Yeah. After reading your previous comment about tagging, I think it'd be a good idea to include this on SearchQuery.

Address review feedback

Harbormaster completed remote builds in B26380: Diff 36475.Jan 31 2024, 2:07 PM

Nice!

services/identity/src/websockets/mod.rs
163–164	This should work

This revision is now accepted and ready to land.Jan 31 2024, 2:13 PM

Replace map_err with ?

This revision was landed with ongoing or failed builds.Jan 31 2024, 2:52 PM

Closed by commit rCOMMf5ae0f430dc6: [identity] [6/n] Requires a Search Query json message from client for user… (authored by will). · Explain Why

This revision was automatically updated to reflect the committed changes.

will added a commit: rCOMMf5ae0f430dc6: [identity] [6/n] Requires a Search Query json message from client for user….

Harbormaster completed remote builds in B26387: Diff 36482.Jan 31 2024, 2:59 PM

[identity] [6/n] Requires a Search Query json message from client for user identity search
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 36475

services/identity/Cargo.toml

services/identity/src/websockets/errors.rs

services/identity/src/websockets/mod.rs

shared/identity_search_messages/src/messages/mod.rs

shared/identity_search_messages/src/messages/search_query.rs

[identity] [6/n] Requires a Search Query json message from client for user identity searchClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 36475

services/identity/Cargo.toml

services/identity/src/websockets/errors.rs

services/identity/src/websockets/mod.rs

shared/identity_search_messages/src/messages/mod.rs

shared/identity_search_messages/src/messages/search_query.rs

[identity] [6/n] Requires a Search Query json message from client for user identity search
ClosedPublic
Actions

Revision Contents
Changeset List