Add type restrictions to Oauth directory #15687

Vici37 · 2025-04-20T04:28:39Z

This is the output of compiling cr-source-typer and running it with the below incantation:

CRYSTAL_PATH="./src" ./typer spec/std_spec.cr --error-trace --exclude src/crystal/ --stats --progress --union-size-threshold 2 --ignore-private-defs src/oauth

This is related to #15682 .

straight-shoota · 2025-04-22T21:00:18Z

src/oauth/authorization_header.cr

@@ -6,7 +6,7 @@ struct OAuth::AuthorizationHeader
    @first = true
  end

-  def add(key, value)
+  def add(key : String, value : String?) : Bool?


suggestion: The return type Bool? is technically correct. But it doesn't make sense.

Bool is inferred from the last expression. It just accidentally leaks an irrelevant internal detail. The return type should be only Nil.

Suggested change

def add(key : String, value : String?) : Bool?

def add(key : String, value : String?) : Nil

straight-shoota · 2025-04-22T21:04:11Z

src/oauth2/access_token/access_token.cr

@@ -2,7 +2,7 @@
 #
 # Use `#authenticate` to authenticate an `HTTP::Client`.
 abstract class OAuth2::AccessToken
-  def self.new(pull : JSON::PullParser)
+  def self.new(pull : JSON::PullParser) : OAuth2::AccessToken


suggestion: Use self to designate this method as a constructor.

Suggested change

def self.new(pull : JSON::PullParser) : OAuth2::AccessToken

def self.new(pull : JSON::PullParser) : self

Technically, this is might not even be necessary because the method name .new implies a constructor. But it's still best to follow the expected type annotation scheme for constructors.

Ditto below.

straight-shoota · 2025-04-22T21:12:44Z

src/oauth2/access_token/mac.cr

@@ -34,7 +34,7 @@ class OAuth2::AccessToken::Mac < OAuth2::AccessToken
    request.headers["Authorization"] = header
  end

-  def self.signature(ts, nonce, method, uri, host, port, ext, mac_algorithm, mac_key) : String
+  def self.signature(ts : Int64 | Int32, nonce : String, method : String, uri : String, host : String, port : Int32 | String, ext : String, mac_algorithm : String, mac_key : String) : String


suggestion: Unions between numeric types should almost always be resolved to either an abstract type like Int or the biggest possible type (to make use of autocasting).

ts is a unix timestamp, so in theory it should probably be UInt64.
But that won't autocast from any signed integer type.

Maybe Int is best then... or leave it unrestrictected?

I only see ts used for its string construction of the normalized_request_string, so unrestricted makes sense, too. I didn't realize ts stood for timestamp, ha. I can remove the restriction on that argument for now.

straight-shoota · 2025-04-23T08:39:35Z

@Vici37 Please avoid force-pushes on PRs with active review, in order to preserve history and make the reviewer's jobs easier (cf. https://github.com/crystal-lang/crystal/blob/master/CONTRIBUTING.md#making-good-pull-requests).

That means, no rebasing. Just keep old commits around and push new ones (which can include merges with master).

Vici37 · 2025-04-24T02:27:37Z

Whoops! Yup, my mistake, sorry about that. Happy to have learned that lesson through a relatively small PR 😅

Sija · 2025-04-24T10:25:23Z

src/oauth/consumer.cr

@@ -72,7 +72,7 @@ class OAuth::Consumer
  # [RFC 5849, Section 2.1](https://tools.ietf.org/html/rfc5849#section-2.1).
  #
  # Raises `OAuth::Error` if there was an error getting the request token.
-  def get_request_token(oauth_callback = "oob") # : RequestToken
+  def get_request_token(oauth_callback : String = "oob") : OAuth::RequestToken


Full path is not needed.

Suggested change

def get_request_token(oauth_callback : String = "oob") : OAuth::RequestToken

def get_request_token(oauth_callback : String = "oob") : RequestToken

Sija · 2025-04-24T10:25:52Z

src/oauth/consumer.cr

@@ -81,7 +81,7 @@ class OAuth::Consumer
  # Returns an authorize URI from a given request token to redirect the user
  # to obtain an access token, as specified by
  # [RFC 5849, Section 2.2](https://tools.ietf.org/html/rfc5849#section-2.2).
-  def get_authorize_uri(request_token, oauth_callback = nil) : String
+  def get_authorize_uri(request_token : OAuth::RequestToken, oauth_callback : String? = nil) : String


ditto

Suggested change

def get_authorize_uri(request_token : OAuth::RequestToken, oauth_callback : String? = nil) : String

def get_authorize_uri(request_token : RequestToken, oauth_callback : String? = nil) : String

Sija · 2025-04-24T10:26:10Z

src/oauth/consumer.cr

@@ -116,7 +116,7 @@ class OAuth::Consumer
  # [RFC 5849, Section 2.3](https://tools.ietf.org/html/rfc5849#section-2.3).
  #
  # Raises `OAuth::Error` if there was an error getting the access token.
-  def get_access_token(request_token, oauth_verifier, extra_params = nil) : AccessToken
+  def get_access_token(request_token : OAuth::RequestToken, oauth_verifier : String, extra_params : Hash(String, String)? = nil) : AccessToken


ditto

Suggested change

def get_access_token(request_token : OAuth::RequestToken, oauth_verifier : String, extra_params : Hash(String, String)? = nil) : AccessToken

def get_access_token(request_token : RequestToken, oauth_verifier : String, extra_params : Hash(String, String)? = nil) : AccessToken

Sija · 2025-04-24T10:27:48Z

src/oauth2/access_token/mac.cr

@@ -34,7 +34,7 @@ class OAuth2::AccessToken::Mac < OAuth2::AccessToken
    request.headers["Authorization"] = header
  end

-  def self.signature(ts, nonce, method, uri, host, port, ext, mac_algorithm, mac_key) : String
+  def self.signature(ts, nonce : String, method : String, uri : String, host : String, port : Int32 | String, ext : String, mac_algorithm : String, mac_key : String) : String


String restriction for port seems a bit odd...

Ha, agreed, good catch. Looks to be caused by this spec that's passing in "4000" for the port

Vici37 force-pushed the add-type-restrictions-to-oauth branch from f7312bf to 110f338 Compare April 20, 2025 04:29

Vici37 mentioned this pull request Apr 20, 2025

Run source-typer on crystal standard library, give or take #15682

Draft

straight-shoota added topic:stdlib:networking kind:chore labels Apr 22, 2025

straight-shoota reviewed Apr 22, 2025

View reviewed changes

Vici37 added 2 commits April 22, 2025 22:15

Add type restrictions to Oauth directory

71ca5fe

Review comments

674981b

Vici37 force-pushed the add-type-restrictions-to-oauth branch from 110f338 to 674981b Compare April 23, 2025 04:16

straight-shoota approved these changes Apr 23, 2025

View reviewed changes

Sija reviewed Apr 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add type restrictions to Oauth directory #15687

Add type restrictions to Oauth directory #15687

Uh oh!

Vici37 commented Apr 20, 2025

Uh oh!

straight-shoota Apr 22, 2025 •

edited

Loading

Uh oh!

straight-shoota Apr 22, 2025

Uh oh!

straight-shoota Apr 22, 2025

Uh oh!

Vici37 Apr 23, 2025

Uh oh!

straight-shoota commented Apr 23, 2025

Uh oh!

Vici37 commented Apr 24, 2025

Uh oh!

Sija Apr 24, 2025

Uh oh!

Sija Apr 24, 2025

Uh oh!

Sija Apr 24, 2025

Uh oh!

Sija Apr 24, 2025

Uh oh!

Vici37 Apr 27, 2025

Uh oh!

Uh oh!

	def add(key : String, value : String?) : Bool?
	def add(key : String, value : String?) : Nil

	def self.new(pull : JSON::PullParser) : OAuth2::AccessToken
	def self.new(pull : JSON::PullParser) : self

	def get_request_token(oauth_callback : String = "oob") : OAuth::RequestToken
	def get_request_token(oauth_callback : String = "oob") : RequestToken

	def get_authorize_uri(request_token : OAuth::RequestToken, oauth_callback : String? = nil) : String
	def get_authorize_uri(request_token : RequestToken, oauth_callback : String? = nil) : String

	def get_access_token(request_token : OAuth::RequestToken, oauth_verifier : String, extra_params : Hash(String, String)? = nil) : AccessToken
	def get_access_token(request_token : RequestToken, oauth_verifier : String, extra_params : Hash(String, String)? = nil) : AccessToken

Uh oh!

Add type restrictions to Oauth directory #15687

Are you sure you want to change the base?

Add type restrictions to Oauth directory #15687

Uh oh!

Conversation

Vici37 commented Apr 20, 2025

Uh oh!

straight-shoota Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

straight-shoota commented Apr 23, 2025

Uh oh!

Vici37 commented Apr 24, 2025

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!

Uh oh!

straight-shoota Apr 22, 2025 •

edited

Loading