OKTA-78337 #15
OKTA-78337 #15
Conversation
larsjohansen-okta
force-pushed
the
lars-78337-advanced-user-search
branch
30 times, most recently
from
67eb431
to
6c97ade
Compare
larsjohansen-okta
force-pushed
the
lars-78337-advanced-user-search
branch
4 times, most recently
from
558324e
to
05f6c97
Compare
| return this; | ||
| } | ||
|
|
||
| private boolean checkComplexity() { |
There was a problem hiding this comment.
I thought we decided to let the server enforce this. We never want to end up in a situation where the server loosens up its constraints (allows more operators) and then have the SDK explicitly precluding use of the new server abilities.
There was a problem hiding this comment.
My understanding was to match the current restrictions, but I can remove it if you think it's better.
There was a problem hiding this comment.
I think it would be fine to doc the current restrictions on the developer site (if we don't already), but enforcing them in the client code is going too far IMO. I think we should remove it and let the server drive the business logic.
There was a problem hiding this comment.
Ok, sounds good to me, in that way we will never get into a situation where we have more strict constraints on the client than the server, which would be really bad.
|
nits + Trevor's comments then lgtm |
| * @return PagedResults<User> The users with paging info. | ||
| * @throws IOException If an input or output exception occurred. | ||
| */ | ||
| public PagedResults<User> getUsersPagedResultsWithAdvancedSearchAndLimitAndAfterCursor(FilterBuilder filterBuilder, int limit, String after) throws IOException { |
There was a problem hiding this comment.
I'd just like to reiterate a couple design principles (from the API manifesto) to keep in mind with any SDK change:
- API Should Be As Small As Possible But No Smaller
- When in doubt leave it out; you can always add but you can never remove
- Public APIs are forever - one chance to get it right
- Minimize Accessibility of Everything
- Names Matter–the API is a Little Language
Here, the name getUsersPagedResultsWithAdvancedSearchAndLimitAndAfterCursor() is very descriptive, but it's almost unwieldy. Also, do we want to call the new functionality "Advanced Search" here or just "Search"? From the client's perspective, there isn't anything "advanced" about it; its just SCIM filter no different than the one it's already using via the filter parameter.
There was a problem hiding this comment.
We've had the convention of calling the q parameter the search functionality (both in code and on developer.okta.com, see http://developer.okta.com/docs/api/resources/users.html#list-users-with-search) so I thought advanced search was the official term we used for this. I've also seen that in the SDK we're using the term query (getUsersWithQuery) more than search, so there's already a base for confusion when reading the SDK and the documentation on developer.okta.com. I thought that it would be a good idea to be consistent with the docs now that we have the chance, instead of making it confusing saying that the new functionality is called search but it's not the search that we document online, it's what we call advanced search online.
Regarding the other part of the name for this method, I agree it is very verbose; I was first thinking of using the name getUsersPagedResultsWithAdvancedSearch and overload this method with all 3 parameter combinations (search, search + limit, search + limit + cursor). I didn't end up doing that to follow the way we name the methods with filtering, which is spelling out the parameters in the name of the function. So I followed consistency in this case.
Let me know what you think we should call it; I think we should get rid of parameter names in the method name, and instead assume they read the javadocs and understands. I would then call them all getUsersPagedResultsWithAdvancedSearch(...).
There was a problem hiding this comment.
Looking at this again and thinking through it more, I like it the way you have it. Thanks for the explanation as well.
larsjohansen-okta
force-pushed
the
lars-78337-advanced-user-search
branch
3 times, most recently
from
0bfef23
to
fec5866
Compare
bacon: test resolves OKTA-78337
larsjohansen-okta
force-pushed
the
lars-78337-advanced-user-search
branch
from
fec5866
to
b8bc35f
Compare
|
🚀 |
| // specified in the search parameter. Note that the results might not yet be up to date, as the most up to date | ||
| // data can be delayed up to a few seconds, so use for convenience. | ||
| FilterBuilder filterBuilder = new FilterBuilder("profile.flightNumber eq \"A415\""); | ||
| List<User> users = userApiClient.getUsersWithSearch(filterBuilder); |
There was a problem hiding this comment.
Do we need to call this a beta feature?
Primary reviewers: @tthompson-okta @lboyette-okta @cwu-okta
Secondary reviewers: @federations-okta @prestonchan-okta @sswaika-okta