[PDE-3241] accept dot notation - #18
Conversation
b490dd7 to
7f737b2
Compare
|
|
||
| connection.headers = headers | ||
| connection.options.timeout = timeout | ||
| connection.options.timeout = 60 |
There was a problem hiding this comment.
We have been experiencing timeout issues with certain Payrix requests. When testing via curl, some requests can still fail even after ~120s. Payrix suggested that including the division ID may improve query performance, and initial testing shows response times improving to ~20s.
To improve reliability and accommodate slower queries, I am increasing the request timeout to 60s for now.
| parts | ||
| .map { |part| "[#{Payrix::Util.camel_case(part)}]" } | ||
| .join | ||
| end |
There was a problem hiding this comment.
The original implementation always wrapped the key with an [], which could lead to double-wrapping when the field was already partially formatted.
The goal of this change is to treat string inputs using dot notation (e.g. "merchant.entity.login.division") as a structured field path. These strings are parsed into an array of segments and then consistently formatted into the expected bracketed query structure.
This ensures:
- Symbol inputs remain supported for simple fields
- Dot notation strings are correctly parsed into nested fields
- Do not unintentionally double-wrapped nested fields
There was a problem hiding this comment.
Pull request overview
Enhances the Payrix search query builder to support nested field paths via dot-notation (e.g., "merchant.entity.login.division") and adjusts HTTP request timeout behavior.
Changes:
- Add dot-notation rendering for search fields in
Payrix::RequestOptions::Search::Atom. - Update Faraday request timeout configuration to 60 seconds.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/payrix/request_options/search/atom.rb | Adds dot-notation support by rendering nested field paths into bracketed query syntax. |
| lib/payrix/http/request.rb | Changes Faraday timeout configuration (currently hard-codes request timeout to 60s). |
Comments suppressed due to low confidence (1)
lib/payrix/request_options/search/atom.rb:66
- Dot-notation fields are split on '.', but there’s no validation that each segment is non-empty. Inputs like "merchant..division" or ".division" will produce empty path components (e.g., "[]") in the rendered query, which is very likely to generate invalid SEARCH filters. Consider tightening
validate_field(only when the field is a dot-notation String) to reject strings with empty segments.
def rendered_field(prefix)
return Payrix::Util.camel_case(@field.to_s) unless @field.is_a?(String) && @field.include?('.')
dot_field(prefix.empty?)
end
def dot_field(unprefixed)
parts = @field.split('.').map { |part| Payrix::Util.camel_case(part) }
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| connection.options.timeout = 60 | ||
| connection.options.open_timeout = timeout |
| end | ||
| end | ||
|
|
||
| context 'when the field is dot notation with empty segments at the binning' do |
| end | ||
| end | ||
|
|
||
| context 'when the field is a dotted string and the prefix is a non empty string' do |
| include Singleton | ||
|
|
||
| def send_http(method, base_url, endpoint, data = {}, headers = {}, timeout = 30) | ||
| def send_http(method, base_url, endpoint, data = {}, headers = {}, timeout = 60) |
There was a problem hiding this comment.
Some queries are taking over 20 seconds, so I felt that 30 seconds might still lead to timeouts. That’s why I increased it to 60 seconds.
There was a problem hiding this comment.
I'm not convinced we should do that, but I'll leave it to you.
There was a problem hiding this comment.
I can reduce it to 30 seconds first and see how the query performs, then increase it again if we still encounter timeouts.
| "#{field}[#{@operator}]=#{@value}" | ||
| if prefix.empty? | ||
| "#{formatted_field}[#{@operator}]=#{@value}" | ||
| elsif formatted_field.start_with?('[') |
There was a problem hiding this comment.
so the dot notation field will be formatted to something like [merchant][entity][login][division][equals] when it formatted to something start with [ then we do not want to add an outter [].
pdmholden
left a comment
There was a problem hiding this comment.
Question: why do we need to accept dot notation?
I think we should bump the version number to 1.2.0. This is not a breaking change, but it is more than a patch. We need to resume updating the version number, which we haven't done in three years.
| include Singleton | ||
|
|
||
| def send_http(method, base_url, endpoint, data = {}, headers = {}, timeout = 30) | ||
| def send_http(method, base_url, endpoint, data = {}, headers = {}, timeout = 60) |
There was a problem hiding this comment.
I'm not convinced we should do that, but I'll leave it to you.
Our current implementation only accepts a single field for filtering, represented as a symbol or string: Here, The division id filter is different because it’s a nested field: Since this is not a single symbol or string, we would need to support either dot notation like: Also, I agree with the version suggestion |
6cf0770 to
61e2cc8
Compare
| include Singleton | ||
|
|
||
| def send_http(method, base_url, endpoint, data = {}, headers = {}, timeout = 30) | ||
| def send_http(method, base_url, endpoint, data = {}, headers = {}, timeout = 60) |
|
|
||
| module Payrix | ||
| VERSION = '1.0.0' | ||
| VERSION = '1.2.0' |
There was a problem hiding this comment.
Oh, maybe this should be 1.1.0. I don't know why I thought it should be 1.2.0. 🤦
There was a problem hiding this comment.
Sorry, I wasn't thinking either. 🙏 ♻️
f591f02 to
74f7ac1
Compare
Summary
Enhance the Payrix search query builder to support nested field paths using dot notation syntax
Changes
Testing
Current search query should continue to work, add the following code in
bin/consolebin/console, it should return a list of txn idsNew search query should work, add the following code in
bin/console