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

request->id validation and request->method delimiter #11

Closed
vanodevium opened this issue Jan 11, 2021 · 8 comments
Closed

request->id validation and request->method delimiter #11

vanodevium opened this issue Jan 11, 2021 · 8 comments

Comments

@vanodevium
Copy link
Contributor

Hello @tabuna
Thanks for this awesome package!

I have two simple questions

  1. According to the specification, id in RPC request described as
id
An identifier established by the Client that MUST contain a String, Number, or NULL value if included. If it is not included it is assumed to be a notification. The value SHOULD normally not be Null [1] and Numbers SHOULD NOT contain fractional parts [2]
The Server MUST reply with the same value in the Response object if included. This member is used to correlate the context between the two objects.

[1] The use of Null as a value for the id member in a Request object is discouraged, because this specification uses a value of Null for Responses with an unknown id. Also, because JSON-RPC 1.0 uses an id value of Null for Notifications this could cause confusion in handling.

[2] Fractional parts may be problematic, since many decimal fractions cannot be represented exactly as binary fractions.

What is the reason for the current validation rule, which is

'id'      => 'regex:/^\d*(\.\d{2})?$/|nullable',
  1. method delimiter
    How can I change default @ to the . (dot) or : (colon)?
@vanodevium vanodevium changed the title request->id validation and method delimiter request->id validation and request->method delimiter Jan 11, 2021
@tabuna
Copy link
Member

tabuna commented Jan 11, 2021

Hi, which rule do you think is more appropriate?

Now there is no way to change the separator:

server/src/Guide.php

Lines 109 to 110 in 92e0e2b

$class = Str::beforeLast($request->getMethod(), '@');
$method = Str::afterLast($request->getMethod(), '@');

But we can make it dynamic through PR

@vanodevium
Copy link
Contributor Author

@tabuna It sounds good. I have few ideas about it:

  • I can use laravel's style config files
  • I can bind sajya.method.delimiter to the application container
  • I can append methodDelimiter property to the Request class

Which one do you prefer?

And about id validation: I think we can use the simple string validation rule. For example, then we will be able to use uuidv4 as request id

@vanodevium
Copy link
Contributor Author

@tabuna ping :)

@tabuna
Copy link
Member

tabuna commented Jan 12, 2021

Hey. I think a good method was to move it to the configuration file. But one single setting seems to be meager.

What do you think about adding a second optional argument?

/**
 * Guide constructor.
 *
 * @param string[] $procedures
 */
public function __construct(array $procedures = [], string $delimiter = '@')

Then we could pass it on.

Route::rpc('/v1/endpoint', [TennisProcedure::class], '@')->name('rpc.endpoint');

Both options are acceptable.

As for validation, we have failed tests with the value string.

@tabuna
Copy link
Member

tabuna commented Jan 12, 2021

And about id validation: I think we can use the simple string validation rule. For example, then we will be able to use uuidv4 as request id

I have verified that the uuid1 and uuid4 IDs passed as a valid parameter. f09d14b

@vanodevium
Copy link
Contributor Author

@tabuna
I did the best that I could do.
#12

@vanodevium
Copy link
Contributor Author

@tabuna Tnx!
One simple question: what about this ability for older versions of this package for laravel 6.x?

@tabuna
Copy link
Member

tabuna commented Jan 13, 2021

Release https://github.com/sajya/server/releases/tag/2.2.0

As for Laravel 6, I have no plans to release a release for that.
See #6 there a guy made a fork

@tabuna tabuna closed this as completed Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants