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

Add custom Jackson (de-) serializer for ZonedDateTime #3198

Merged
merged 2 commits into from Dec 14, 2016

Conversation

Projects
None yet
2 participants
@joschi
Contributor

joschi commented Dec 13, 2016

Description

This PR adds a custom serializer and deserializer for ZonedDateTime to the ObjectMapper used by MongoJack, so that properties with type ZonedDateTime are properly serialized as a MongoDB date field.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the code style of this project.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@joschi joschi added this to the 2.2.0 milestone Dec 13, 2016

@joschi joschi referenced this pull request Dec 13, 2016

Closed

Support for multiple retention strategies #2880

23 of 27 tasks complete
@bernd

This comment has been minimized.

Member

bernd commented Dec 13, 2016

Nice! We need the same for Joda DateTime because that is also wrongly serialized as a String into MongoDB.

@joschi

This comment has been minimized.

Contributor

joschi commented Dec 13, 2016

@bernd I wouldn't use JodaTime (DateTime) in new classes and migration of existing entries is (unfortunately) not that easy.

But we can look into this in another PR (possibly post-2.2.0).

@bernd

This comment has been minimized.

Member

bernd commented Dec 14, 2016

@joschi I think we have to discuss the usage Joda Time vs Java Core Time at some point. Right now we are using both (also for new code) which is confusing. But that's unrelated to this PR.

What is the problem with migration? We already have the problem that we need to migrate data after this. We are using it for the index set config which is now in a public beta release and we are also using this in Graylog Enterprise.

@bernd bernd self-assigned this Dec 14, 2016

@joschi

This comment has been minimized.

Contributor

joschi commented Dec 14, 2016

What is the problem with migration? We already have the problem that we need to migrate data after this.

I'd argue that given this is still a beta, we don't need to care for a migration process for the index_sets collection. It's not worth the effort.

@bernd

This comment has been minimized.

Member

bernd commented Dec 14, 2016

I'd argue that given this is still a beta, we don't need to care for a migration process for the index_sets collection. It's not worth the effort.

But what is the effort? 😃 What needs to be done to migrate this?

@joschi

This comment has been minimized.

Contributor

joschi commented Dec 14, 2016

@bernd Read all entries, check the type of the creation_date field, update the document in case it was a string.

Sounds not like much but it's still code only required for people who ran Graylog 2.2.0-beta.1 or 2.2.0-beta.2 and we'll have to maintain it. I'd rather concentrate on other things. 😉

joschi added some commits Dec 13, 2016

@joschi joschi force-pushed the mongojack-jsr-310 branch from df7064f to 467baaf Dec 14, 2016

@bernd

This comment has been minimized.

Member

bernd commented Dec 14, 2016

So it turned out that only one collection in Graylog Enterprise needs a migration and others will be fixed on write and/or are not sorted/queried by date anywhere.

@bernd

bernd approved these changes Dec 14, 2016

LGTM and tested with several collections.

@bernd bernd merged commit 2835de9 into master Dec 14, 2016

4 checks passed

ci-web-linter Jenkins build graylog-pr-linter-check 1144 has succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@bernd bernd deleted the mongojack-jsr-310 branch Dec 14, 2016

@bernd bernd removed the ready-for-review label Dec 14, 2016

bernd added a commit that referenced this pull request Dec 14, 2016

Remove java.util.Date workaround now that we correctly serialize Date…
…Time

Serialization of DateTime has been fixed in #3198.

joschi added a commit that referenced this pull request Dec 14, 2016

Remove java.util.Date workaround now that we correctly serialize Date…
…Time (#3202)

Serialization of DateTime has been fixed in #3198.

joschi added a commit that referenced this pull request Dec 15, 2016

Remove TODO about ZonedDateTime in IndexSetConfig
The issue has been resolved in #3198

joschi added a commit that referenced this pull request Dec 15, 2016

Remove TODO about ZonedDateTime in IndexSetConfig
The issue has been resolved in #3198

joschi added a commit that referenced this pull request Dec 16, 2016

Remove TODO about ZonedDateTime in IndexSetConfig
The issue has been resolved in #3198

joschi added a commit that referenced this pull request Dec 19, 2016

Remove TODO about ZonedDateTime in IndexSetConfig
The issue has been resolved in #3198

joschi added a commit that referenced this pull request Dec 20, 2016

Remove TODO about ZonedDateTime in IndexSetConfig
The issue has been resolved in #3198

bernd added a commit that referenced this pull request Dec 21, 2016

Address open TODOs for Graylog 2.2.0 (#3211)
* Remove TODO about ZonedDateTime in IndexSetConfig

The issue has been resolved in #3198

* Migrate remaining index settings from config file to IndexSetConfig

* elasticsearch_analyzer
* elasticsearch_template_name
* index_optimization_max_num_segments
* disable_index_optimization

* Allow overriding analyzer in MessageResource#analyze()

* Split update of default index set into its own migration

* Remove index template from IndexSetUpdateRequest

* Ignore unknown properties in IndexSetConfig

Deserializing used to fail because of the removed "default" field.

* Use correct timestamp in V20161216123500_DefaultIndexSetMigration

* Remove index analyzer from IndexSetUpdateRequest

* Add index settings to Index Set configuration form

* Properly handle index_optimization_max_num_segments and index_optimization_disabled

* Don't allow users to set the index template name

* Use fixed index template name

The index set config ID isn't available at the time of creation.

* Fix IndexSetsResourceTest

* Use AutoValue builder style in IndexSetUpdateRequest#toIndexSetConfig()

* Use AutoValue builder style in IndexSetSummary#toIndexSetConfig()

* Handle missing values in IndexSetConfig in @JsonCreator

* Add migrated indexing settings to all existing index sets

* Fix V20161116172100_DefaultIndexSetMigration

The builder for IndexSetConfig requires all settings (in contrast to the IndexSetConfig#create() method).

* Add notes about Exposed Configuration to UPGRADING.rst

[ci skip]

* Use derived template name in V20161216123500_DefaultIndexSetMigration

* Special handling of default index set in V20161216123500_DefaultIndexSetMigration

* Remove @JsonIgnoreProperties from IndexSetConfig

Was "needed" due to a broken migration during development.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment