Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

added new constructor to provide custom UploadIdFactory #17

Closed
wants to merge 1 commit into from
Closed

added new constructor to provide custom UploadIdFactory #17

wants to merge 1 commit into from

Conversation

przemekc
Copy link

I added this to provide custom UploadIdFactory because there is no other way to provide custom instance.

@coveralls
Copy link

coveralls commented Dec 23, 2018

Coverage Status

Coverage increased (+0.002%) to 96.636% when pulling be0b772 on przemekc:master into c3a8934 on tomdesair:master.

@tomdesair
Copy link
Owner

Hi @przemekc,

Thank you for the contribution! Could you explain to me in which way you customized your UploadIdFactory?

Depending on the use cases, we might need to adjust more code in order to support more different implementations of a ID factories.

@przemekc
Copy link
Author

Hi

Thank you for the contribution! Could you explain to me in which way you customized your UploadIdFactory?

I need to provide other way to create UUID.
In my use case I need to get timestamp from UUID but when I use UUID.randomUUID timestamp() method throw Exception (Not a time-based UUID).
So I overide method createId in UploadIdfactor and I'm using Generators.timeBasedGenerator().generate() from com.fastxml.uuid.

That's all I need for now. But without that change there is no way to achive my use case.

I think extract interface for UploadIdFactory is a quite good solution.

@tomdesair
Copy link
Owner

tomdesair commented Dec 23, 2018

I took some time to experiment with refactoring the code to also support other upload ID formats that are not UUID-based. The result of this can be found in PR #18.

The main changes are:

  • UploadIdFactory became an abstract class
  • TusFileUploadService received an additional method withUploadIdFactory where you can provide your own implementation: link
  • The previous ID factory is now called UUIDUploadIdFactory: link
  • As an example, I added a TimeBasedUploadIdFactory: link

@przemekc, would this also cover your requirements?

The thing I like about this approach is that people can choose other ID formats than UUID.

@tomdesair
Copy link
Owner

Note that in that PR UploadInfo only holds a UploadId instance and no longer a UUID. And that UploadId just contains a (URL safe) String.

@przemekc
Copy link
Author

@przemekc, would this also cover your requirements?

The thing I like about this approach is that people can choose other ID formats than UUID.

Yes, It look nice and yes it meets my requirements.
Thanks a lot.

@przemekc
Copy link
Author

przemekc commented Feb 8, 2019

Beacuse #18 has been merged this isn't usefull anymore

@przemekc przemekc closed this Feb 8, 2019
@tomdesair
Copy link
Owner

Hi @przemekc,

I still wanted to thank you again for all your input. I've released the support for custom UploadIdFactory instances of #18 to version 1.0.0-2.0 in Maven Central.

Have a nice weekend!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants