Make sure the directory sync services returns consistent object types by ams11 · Pull Request #148 · workos/workos-ruby

ams11 · 2022-04-11T23:12:56Z

Closes #147

#147 reports an inconsistency in the data types that the Ruby SDK returns for the DirectorySync module:

the list_users endpoint returns an Array of WorkOS::DirectoryUser objects, however, retrieving an individual user record via the get_user endpoint simply returns a Hash of the attributes of the user.
similar discrepancy exists for the list_groups and get_group endpoints.

The unfortunate result of this discrepancy is that we need to write different code in order to access attributes on the objects returned by the two endpoints. e.g. for the username field:

from the list_users result, access as an object property: results.data.first.username
from the get_user result, access as a value in a Hash: result["username"]

Making a change here to have all the endpoints return their results as objects. In order to support backwards compatibility for the Hash-style access (which is currently required to access attributes on the results of the get_user and get_group calls), adding a [] method to both classes, which maintains Hash-style access for the models' attributes.

Long term, we'd like to standardize on always treating the results as objects, not Hashes, so the Hash-style accessors will eventually be removed. Currently, using them will output a warning, noting the upcoming deprecation.

codecov-commenter · 2022-04-12T06:04:05Z

Codecov Report

Merging #148 (eb35f99) into main (4d2edf3) will decrease coverage by 0.60%.
The diff coverage is 84.00%.

@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
- Coverage   98.29%   97.68%   -0.61%     
==========================================
  Files          38       39       +1     
  Lines         822      865      +43     
==========================================
+ Hits          808      845      +37     
- Misses         14       20       +6

Impacted Files	Coverage Δ
lib/workos/directory_user.rb	`96.42% <75.00%> (+0.42%)`	⬆️
lib/workos/directory_group.rb	`95.00% <80.00%> (+0.88%)`	⬆️
lib/workos/deprecated_hash_wrapper.rb	`83.33% <83.33%> (ø)`
lib/workos.rb	`100.00% <100.00%> (ø)`
lib/workos/directory_sync.rb	`100.00% <100.00%> (ø)`

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

mthadley · 2022-04-13T00:47:16Z

+      # Warn the user not to use the Hash-style access for attributes.
+      warning_message = "WARNING: The Hash style access for DirectoryUser attributes is deprecated and will be removed
+in a future version. Please use user.#{attribute_name} instead of user['#{attribute_name}']"
+      puts warning_message


I think we want to use warn instead of puts for logging warnings.

Looks like there's also a category option and a :deprecated value that can be passed to warn, but I'm not sure if that option is supported on some of the older Ruby versions we apparently support.

ams11 · 2022-04-13T06:23:37Z

    extend T::Sig

-    attr_accessor :id, :name
+    attr_accessor :id, :name, :custom_attributes, :raw_attributes


I noticed that DirectoryUser exposes :custom_attributes and :raw_attributes through attr_accessor, so adding them here as well for consistency. I was originally planning to exclude these two from the hash, but since they're available for access on DirectoryUser, I figured it would be safer to keep them (and add them to DirectoryGroup for consistency).

…o all methods

mthadley

@ams11 LGTM! Thanks for the work on this one.

I left a few more comments, but they aren't blockers for me. Only thing I think we should do now is split out the version bump from these changes to the API.

mthadley · 2022-04-14T19:40:05Z

      }
    end

+    def to_hash


I think the idiomatic naming for this method would be to_h, similar to Array#to_h.

Also, do we want to be adding a to_h to the public API for just these objects? I'd be fine if we left this out, and didn't mention hash conversion in our warning message.

I went back and forth on to_h vs to_hash and eventually settled on to_hash, imitating the one on the Hash itself - https://apidock.com/ruby/Hash/to_hash (and because it's marginally more descriptive). But I don't have a strong opinion on this one way or another - happy to change to to_h, if you think that's better/more consistent.

As far as leaving it out altogether, I'm a little uncomfortable with that - if somebody was using a Hash method on the result, then all of a sudden, we don't really have support for it. This way, they at least have an easy work around. Counter-argument: we can just suggest that they call to_json as a workaround (which is what I'm doing here anyway), but that starts to feel a bit hacky. Also, converting the User or Group info into a Hash kinda makes sense to me - we start off with basically a Hash of data from the IdP, doesn't seem like that much of a stretch to give them a direct way to convert it back to a Hash?

hmm, well Hash implements both to_h and to_hash, and between the two, it sounds like to_h is the more generally applicable method. But I don't feel strongly about this either, so go with your gut.

I'm still not convinced that we need to support to_hash going forward. Isn't it part of the DeprecatedHash API?

In either case, we can punt on making a decision here until we are ready to remove DeprecatedHash completely.

👍 that's compelling enough :), went ahead and switched to using to_h. As far as whether we need it or not, we can't quite punt until removing DeprecatedHash, or at least not without potentially breaking users again 😭 - my current warning message suggests using to_h to get a Hash, if you're trying to use a Hash method on the results... so it wouldn't be great if we removed support for the method we were telling people to use 😅.

Isn't it part of the DeprecatedHash API

I currently have the method independent of DeprecatedHash - DirectoryGroup and DirectoryUser both implement it, so I've not really been treating as a part of DeprecatedHash, as much as a backwards compatibility helper for anyone who really wants to be able to get the results as a Hash.

Ah, good point.

I think there's two options here:

We consider to_h as deprecated, and remove it when we remove DeprecatedHash, since none of the other resource objects (WorkOS::Profile, etc.) implement a to_h. So then we would also want to update that warning message now, and avoid tell them to use to_h.

We keep to_h here, but then we also implement it on the rest of the resource objects as well.

So, my goal here is that we don't end up with a different kind of inconsistency in our API with DirectoryUser and DirectoryGroup being the only classes that implement to_h,

Does that make sense?

My initial preference was for option 1 since it seemed like less work, but I would be fine with option 2 as well.

Yeah, that 💯 makes sense... as for the options ... option 1 is definitely less work, but 🤔 I still feel bad for taking the Hash away after users have potentially been using it this whole time. So, I guess I prefer option 2...

Though that also commits us to implementing a to_h method any time we add a new class in the future, which is also annoying and liable to end up with inconsistent behavior if we forget to. So, I'm waffling on my preferences a bit.

Though that also commits us to implementing a to_h method any time we add a new class in the future, which is also annoying and liable to end up with inconsistent behavior if we forget to.

Probably wouldn't be a bad idea to refactor those classes, and maybe have them all inherit from a base class that provides to_h, etc.

But yeah, we'd have to commit to doing that work.

👍 to a shared base class to include the to_h implementation... though it still doesn't prevent future classes from showing up that don't inherit from the base class 😭. But we can probably add tests and/or linters to try and catch that.

Either way, I think it's worth shipping this PR as it is now, then coming back with a separate one to add to_h to everything else - does that seem reasonable to you?

Either way, I think it's worth shipping this PR as it is now, then coming back with a separate one to add to_h to everything else - does that seem reasonable to you?

That sounds good! I think we are good to go here.

mthadley · 2022-04-14T19:42:32Z

+
+    def object_name
+      case self
+      when WorkOS::DirectoryUser


It's a little inverted to have a superclass know about the specific classes that inherit from it.

We could have DeprecatedHash have a default implementation like def object_name = "object", and the other classes can override it with their specific name.

However, given we don't plan on supporting this going forward, not a big deal if we stick with this for simplification of removal later.

Hmmm, good point... I can just make it more generic. We'll end up with directory_user instead of user in the warning message, but I think that's ok(?)

mthadley · 2022-04-14T19:45:53Z


 module WorkOS
-  VERSION = '2.2.0'
+  VERSION = '2.2.1'


Do you mind submitting these version changes in a separate PR after we get these behavior changes merged?

That's usually how we do it.

mthadley · 2022-04-14T21:52:55Z

+    # call the original implementation of :replace in Hash,
+    # so we don't show the deprecation warning
+    def replace_without_warning(new_hash)
+      method(:replace).super_method&.call(new_hash)


This is a very pragmatic solution. Love it. 🎉

robframpton · 2022-04-15T18:18:32Z

@ams11 @mthadley are we good to merge this in?

ams11 · 2022-04-15T19:16:04Z

@ams11 @mthadley are we good to merge this in?

Yup! Unless any last second issues turn up, I'm planning to merge this in today. I'll then start a separate, follow-up PR to address #148 (comment), and once that's in, I think we can go ahead and publish a new gem?

Alex Slepak added 12 commits April 11, 2022 16:09

consistent return types, with backwards compat

864a76d

update return types

096b916

update attribute accessors

3ba56fd

update tests

339d923

use single quotes :(

7a72a45

reduce complexity... sort of

2102ab4

use to_json

209a73e

update gem version

6ba0ad0

correct quotes

39bd04e

use to_json for groups also

d528d3c

log warning about using hash-style attribute accessors

2188540

shorten lines

a1dd1c6

ams11 marked this pull request as ready for review April 12, 2022 06:18

ams11 requested review from a team April 12, 2022 06:18

mthadley reviewed Apr 12, 2022

View reviewed changes

Comment thread lib/workos/directory_sync.rb

mthadley reviewed Apr 12, 2022

View reviewed changes

Comment thread lib/workos/directory_user.rb Outdated

try inheriting from Hash

acc600f

mthadley reviewed Apr 13, 2022

View reviewed changes

don't remove custom and raw attributes

2681f5c

ams11 commented Apr 13, 2022

View reviewed changes

Alex Slepak added 8 commits April 13, 2022 15:46

add a DeprecatedHashWrapper base class and inject warning message int…

6507f4e

…o all methods

make linter updates

a837c43

fix linter warnings some moar

d4336d8

change one more

51c7d09

look for new base class outside of module

4480490

add autoload

4665cb7

move base class into WorkOS module

ba14c9a

need autload after all

1e3eb3c

Alex Slepak added 4 commits April 14, 2022 12:31

improve error messages

f5d5797

handle nil

d7ca845

cleanup lint warnings

0586523

generalize replace_without_warnign and have Group model call it too

40fa183

mthadley approved these changes Apr 14, 2022

View reviewed changes

mthadley reviewed Apr 14, 2022

View reviewed changes

Alex Slepak added 14 commits April 14, 2022 15:08

use demodulize

98f2b13

reuse class_name

a2e0057

revert version changes

ea45133

don't use demodulize

6cebca9

clean up

2f41833

implement underscore locally

2188706

use more single quotes

187d3d8

improve regex matchers

72b7913

clean up some more

22c8eeb

add new line

eaf5ad1

fix demodulize method

d55c8c0

fix quotes

6ce4209

add comment

1d560ef

fix typo

26f7f46

mthadley approved these changes Apr 15, 2022

View reviewed changes

mthadley mentioned this pull request Apr 15, 2022

Updates Gemlock to 2.2.1 #151

Closed

Merge branch 'main' into return_types

eb35f99

ams11 merged commit 162bb75 into main Apr 15, 2022

ams11 deleted the return_types branch April 15, 2022 20:58

This was referenced Apr 15, 2022

When working with users from a directory, list_users and get_user return different objects. #147

Closed

Add hash support to all models that represent parsed objects #152

Merged

ams11 mentioned this pull request Apr 27, 2022

Create 2.2.1 version of the workos gem #154

Merged

Conversation

ams11 commented Apr 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mthadley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robframpton commented Apr 15, 2022

Uh oh!

ams11 commented Apr 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

ams11 commented Apr 11, 2022 •

edited

Loading

codecov-commenter commented Apr 12, 2022 •

edited

Loading