You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by simplesteph <gi...@git.apache.org> on 2016/10/14 06:59:52 UTC
[GitHub] nifi pull request #1136: [NIFI-2900] fixes timestamps to be ISO 8601 complia...
GitHub user simplesteph opened a pull request:
https://github.com/apache/nifi/pull/1136
[NIFI-2900] fixes timestamps to be ISO 8601 compliant, as it should b\u2026
\u2026e because of OpenAPI specs
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/simplesteph/nifi NIFI-2900
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/1136.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1136
----
commit 9d0f9a9625cbd4cd9b428888e9695b8929087a81
Author: simplesteph <st...@gmail.com>
Date: 2016-10-14T06:57:41Z
[NIFI-2900] fixes timestamps to be ISO 8601 compliant, as it should be because of OpenAPI specs
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1136: [NIFI-2900] fixes timestamps to be ISO 8601 compliant, as ...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/1136
@mike42 - Thanks for checking out Apache NiFi and looking to make improvements here. We just released version 1.0.0 so we don't have another major release scheduled in the immediate future. We could certainly look to update the endpoints to return these new values in addition to the existing ones (which could then be marked as deprecated). Or we could investigate a means for versioning the API so that existing clients wouldn't break and new clients could utilize the new endpoints. Though it may make sense to do this work when we add i18n support throughout the application which will likely require either a major NiFi version change or supporting multiple versions of the REST API concurrently.
Regardless what additional work we identify here, I believe the work that was discussed with @simplesteph needs to happen as the documentation is currently misreporting the actual data type of these fields.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1136: [NIFI-2900] fixes timestamps to be ISO 8601 compliant, as ...
Posted by simplesteph <gi...@git.apache.org>.
Github user simplesteph commented on the issue:
https://github.com/apache/nifi/pull/1136
I think then we need to add `dataType = "string"` in the annotation to every field `DateTime` that uses any `TimeAdapter` or `DateTimeAdapter`. Hopefully this way the format won't be shown as "date-time". Somehow my maven is acting up and I can't test it...
Let me know your thoughts
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1136: [NIFI-2900] fixes timestamps to be ISO 8601 compliant, as ...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/1136
Our use of swagger is simply for generating the documentation. The swagger.json is generated into the target directory and is not made available as part of the released source or convenience binaries.
I think the issue here is the inability of Swagger to understand the use of JAXB adapters. These values are String's already as the adapters support (un)marshalling them. I would guess that Swagger is simply reading the type of the property as a `java.util.Date` and marking it a date-time.
I would suggest we update the `ApiModelProperty` annotation of all fields that use any JAXB adapters to accurately reflect the actual data type a client can expect. It looks like there is a dataType field in the annotation. I'm not certain but I would expect this to override the data type in the method signature.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1136: [NIFI-2900] fixes timestamps to be ISO 8601 compliant, as ...
Posted by simplesteph <gi...@git.apache.org>.
Github user simplesteph commented on the issue:
https://github.com/apache/nifi/pull/1136
hi @mcgilman , pain commit but it should be good. the resulting `swagger.json` doesn't have any datetime, which makes the api compliant in that regard. Not sure when 2.0 is planned, and not sure how you track todos against it, but converting these fields back to compliant datetime is a must for the next version of the api
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1136: [NIFI-2900] fixes timestamps to be ISO 8601 compliant, as ...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/1136
Hey sorry, I was traveling yesterday. I don't see your most recent changes. Can you double check that you've pushed them?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1136: [NIFI-2900] fixes timestamps to be ISO 8601 complia...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/1136
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1136: [NIFI-2900] fixes timestamps to be ISO 8601 compliant, as ...
Posted by mike42 <gi...@git.apache.org>.
Github user mike42 commented on the issue:
https://github.com/apache/nifi/pull/1136
@mcgilman- I would also like to see the REST API change in the next major release. Date localisation should be moved to the front-end, as only the browser knows the user's locale and timezone.
Noting your comments above, would you be open to reviewing a pull request which corrects the API to use ISO timestamps, and also introduces `moment.js` in the front end to display localised timestamps?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1136: [NIFI-2900] fixes timestamps to be ISO 8601 compliant, as ...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/1136
Thanks @simplesteph! This has been merged to master (without the changes to the TimeAdapter). I've created a JIRA [1] to revisit the entirety of the Rest Api with regards to the OpenApi spec when possible.
[1] https://issues.apache.org/jira/browse/NIFI-2978
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1136: [NIFI-2900] fixes timestamps to be ISO 8601 compliant, as ...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/1136
NiFi follows the semantic versioning guidelines. What your suggesting is changing the meaning of those fields. They are strings that are meant to be human-readable (one with the time and the other with the date and time). If we change them, we could be affecting any existing clients of that API that expect them to be human-readable strings. The clients I'm referring is anyone consuming this API in an existing version of NiFi. As you mentioned, in our case we would need to update the UI (which is a client of this API) to reformat it as necessary. This is not something we would expect others to do in a minor/incremental version change or without somehow versioning the API.
As I mentioned earlier, I believe that any field using a JAXB adapter is currently being interpreted incorrectly by Swagger. While it would be nice if Swagger could correctly determine the actual data type (a string in this case and not a date-time) it appears they at least provide a mechanism for setting it explicitly. We should be taking advantage of that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1136: [NIFI-2900] fixes timestamps to be ISO 8601 compliant, as ...
Posted by mcgilman <gi...@git.apache.org>.
Github user mcgilman commented on the issue:
https://github.com/apache/nifi/pull/1136
Hey @simplesteph, thanks for the PR!
I'm not sure we can merge this in as is. I'm afraid that changing the format of the of pre-existing values (like all time values in this case) could break existing clients.
We do have two separate adapters for `java.util.Date`'s. The purpose of these adapters is to create human readable values for either a timestamp or for a date-time. The timestamps are typically used to show when something was latest refreshed. Due to the frequency, rendering the date in these scenarios is not necessary. The date-time adapter will additionally include the date and is typically used for items like events.
Admittedly, I'm not super familiar with the OpenAPI spec. Rather than changing these values, would it help if we updated the documentation to better set the expectation of what's being returned?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1136: [NIFI-2900] fixes timestamps to be ISO 8601 compliant, as ...
Posted by simplesteph <gi...@git.apache.org>.
Github user simplesteph commented on the issue:
https://github.com/apache/nifi/pull/1136
That could be a solution, but I believe it's a more complicated one than just keeping the date-time annotation and make the API compliant with the OpenAPI specs. If you create a "Time" class then it could work. I looked and it seems that yes, Swagger doesn't take JAXB adapters into account.
I'm still wondering if it wouldn't be simpler and better to just keep it as date-time and update the UI to re-format it if need be. Which clients would be broken by this change?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1136: [NIFI-2900] fixes timestamps to be ISO 8601 compliant, as ...
Posted by simplesteph <gi...@git.apache.org>.
Github user simplesteph commented on the issue:
https://github.com/apache/nifi/pull/1136
Hi,
I understand the concern. The tests were passing on my end, but maybe something has been broken.
Let me explain the rationale:
1) Your API should comply with the OpenAPI specs as you're using swagger and date-time (see https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md). A datetime looks like this: https://xml2rfc.tools.ietf.org/public/rfc/html/rfc3339.html#anchor14. Example: 1985-04-12T23:20:50.52Z or 1996-12-19T16:39:57-08:00. Knowing this, two solutions: either you rename your variable to "string" (and then the expectations are set), or if you keep date-time, you need to merge my changes.
2) Regarding the UI: in my PR, I chosen to have timestamps with zones, meaning the timestamp will be adapted from where the NiFi instance is running (so not UTC). That's a good first step. The second step is to have the UI website format the date it's receiving from the now-valid REST API, and that's probably something we may want to add in this PR, or in a separate one. FYI- I have tested the UI and things aren't broken as far as I can see
3) For breaking the existing clients: which clients are you talking about?
Looking forward to reading from you
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1136: [NIFI-2900] fixes timestamps to be ISO 8601 compliant, as ...
Posted by simplesteph <gi...@git.apache.org>.
Github user simplesteph commented on the issue:
https://github.com/apache/nifi/pull/1136
Sorry I haven't had the time to work on it just yet... Agree with all the above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---