-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Source Facebook + Google search console: after remove end_date
param connector is not updated and don't throw error
#8060
Comments
@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
|
Implements better error catching in #8029. |
@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
param connector is not updated and don't throw errorend_date
param connector is not updated and don't throw error
From what I see this issue has almost nothing to do with the connector. First, there is inconsistency in the payload sent to
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 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 |
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 For the specific |
I'm in favor of treating empty optional string fields as |
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 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
and customer entered empty string PSEUDO-CODE:
|
@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? |
Enviroment
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.
The text was updated successfully, but these errors were encountered: