Closed marcosmarxm closed 1 year ago
@marcosmarxm from the logs I can see that error is thrown in the Core, not in the connector's code. The spec looks correct to me. Maybe UI issue as well.
it does seem like a UI issue where the UI shold exclude an empty field from being sent to the backend
@jrhizor assigning to you for triage
@sherifnada UI shouldn't exclude empty fields to be sent to the backend, should it? What if we entered some input that is a string but then cleared it - I believe we should send an empty string for such case. The same is here.
I believe It is a backend problem, where an empty string is assumed as a not correct value. If we start to exclude empty strings - it may break some other connectors.
I would even say that we probably should send empty values always instead of setting them to undefined, null for strings or totally omitting them - it will be easier to notice any errors when fields was somehow omitted
Actually it also looks like there is some issue on backend here: I tried to call it with the same schema and it looks like it passes validation step as with end_date: ""
I get check_connection failure with message: "Check Connection Failed!"
2021-11-30 10:01:19 INFO TemporalAttemptExecution(get):116 - Executing worker wrapper. Airbyte version: 0.32.8-alpha-cloud
Implements better error catching in #8029.
@jrhizor @Jamakase should this be closed since #8029 has been merged?
@marcosmarxm can you verify that this is working as expected now?
@jrhizor I was able to update the value, but not to remove the field value.
@sherifnada is it safe to assume that we never have empty strings (or if there are then we can send a null)? This would make it much easier for the frontend.
I think it is true since it'd be marked as required. I don't think we should have a required field that permits empty strings.
@jrhizor in this case the field is optional right? so if it's an optional field, we can assume empty string is safely replaced with null. The same is probably true for a required field? i'm less certain on the 2nd one
This error also happens for the Google search console connector, which exposes an end-date
parameter too.
From what I see this issue has almost nothing to do with the connector.
First, there is inconsistency in the payload sent to /api/v1/scheduler/sources/check_connection
.
When omitting the end_date
field in the UI, it is also missing in the payload:
When filling in the end_date
param and then returning back to an empty string, the key end_date
is present in the payload:
and this leads to an error:
Request to /api/v1/sources/check_connection_for_update
is also sent with an empty string value for the end_date
key:
This seems like an issue with editing the field in the UI.
Second, config validation made in java seems to be different from the one made in python, because python is ok with an empty string against {'format': 'date-time'}
field.
I suggest sending null instead of empty string as @jrhizor and @sherifnada mentioned earlier would fix the problem.
The only thing I would change in the connector's specs is the validation pattern, which is obviously not the best choice.
Hi @marcosmarxm could you please have a look ta my comment above? I guess the fix should be done on the platform UI side, not the connector
Resurfacing this for the frontend team based on the latest investigation above @timroes
That's an interesting problem, and I am not entirely sure what's the desired fix should be here. Is an empty input box a null
(or not present in the object) or an empty string? In general I'd suggest empty input fields should be empty strings (since otherwise you'd also not have any way of specifying empty string).
For the specific format
: date-time
I agree that if left empty we might want to rather treat it as null
(and later on anyway have a proper datepicker for them). Since we're already working on improving the connector components (and datepicker would be part of this) and this issue is already open since quiet some time: @misteryeo what is your priority and urgency for this? Should we try to quick fix this still before the datepicker, or does this have another month to wait for a proper datepicker?
I'm in favor of treating empty optional string fields as null
- i can't think of a case where a connector would specifically want empty string but break on null
Issue is still present for Facebook Marketing
@timroes @edmundito did the frontend team have a level of effort on this already? trying to do some guesstimates for Q4 prios
@sherifnada @edgao
This is a real problem for a lot of connectors which has optional date parameter(s):
source-facebook-marketing
, source-google-ads
, source-mixpanel
, etc
For example we are struggling on this oncall https://github.com/airbytehq/oncall/issues/503 which still does not work
My proposal:
if we have such non-required field
{
"start_date": {
"type": "string",
"format": "date-time"
}
}
and customer entered empty string ""
UI has to send config.json
to server WITHOUT key "start_date" at all
PSEUDO-CODE:
if config["start_date"] == "" and config["start_date"] is not required and config["start_date"] has format:
config.pop("start_date")
@lshrinivas that should be rather low level of effort. The largest part of effort would be testing that this change doesn't break anything else.
Any news in regards to this issue?
Current Behavior
After creating a Facebook Marketing source I'm not able to remove the
end_date
params. If I try the UI return me to the source page, give me the impression it worked... but when I return to the connector setting page the end date param still there.Expected Behavior
Tell us what should happen.
Logs
Response from check_connection_for_update endpoint:
Steps to Reproduce
Create a Facebook Marketing source (using integration test creds)
After try to remove the end date params
Are you willing to submit a PR?
Remove this with your answer.