Skip to content
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

fix: Normalize header handling for all HeaderInit types #203

Merged
merged 3 commits into from
Feb 20, 2025

Conversation

AlexanderSlaa
Copy link
Contributor

Description:
This pull request addresses an issue where filtering custom headers using Object.entries(options.headers) fails when the headers are not provided as a plain object (for example, when using a Headers instance or an array of key-value pairs).

Changes Introduced:

  • Added a new utility function normalizeHeaders that converts any valid HeaderInit (i.e. a Headers instance, an array of key-value pairs, or a plain object) into a plain object.
  • Updated the custom header filtering logic to work reliably by first normalizing options.headers before filtering out the default headers.
  • Included JSDoc documentation for the normalizeHeaders function.

Testing & Impact:

  • Manually verified that the new implementation correctly processes different header types.
  • Ensured that the filtering logic now works uniformly regardless of the header format.
  • This improvement ensures better compatibility with the Fetch API and reduces potential errors when using custom headers.

@StevenSlaa
Copy link

@BruceMacD Could you take a look at this?

Copy link
Collaborator

@BruceMacD BruceMacD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this, and sorry for the delayed response.

This is looking good, but can we also add a test in test/utils.test.js to document this behavior?

Something like this:

import { describe, it, expect, vi, beforeEach } from 'vitest'
import { get } from '../src/utils'

describe('get Function Header Tests', () => {
  const mockFetch = vi.fn();
  const mockResponse = new Response(null, { status: 200 });

  beforeEach(() => {
    mockFetch.mockReset();
    mockFetch.mockResolvedValue(mockResponse);
  });

  const defaultHeaders = {
    'Content-Type': 'application/json',
    'Accept': 'application/json',
    'User-Agent': expect.stringMatching(/ollama-js\/.*/)
  };

  it('should use default headers when no headers provided', async () => {
    await get(mockFetch, 'http://example.com');
    
    expect(mockFetch).toHaveBeenCalledWith('http://example.com', {
      headers: expect.objectContaining(defaultHeaders)
    });
  });

  it('should handle Headers instance', async () => {
    const customHeaders = new Headers({
      'Authorization': 'Bearer token',
      'X-Custom': 'value'
    });

    await get(mockFetch, 'http://example.com', { headers: customHeaders });

    expect(mockFetch).toHaveBeenCalledWith('http://example.com', {
      headers: expect.objectContaining({
        ...defaultHeaders,
        'authorization': 'Bearer token',
        'x-custom': 'value'
      })
    });
  });

  it('should handle plain object headers', async () => {
    const customHeaders = {
      'Authorization': 'Bearer token',
      'X-Custom': 'value'
    };

    await get(mockFetch, 'http://example.com', { headers: customHeaders });

    expect(mockFetch).toHaveBeenCalledWith('http://example.com', {
      headers: expect.objectContaining({
        ...defaultHeaders,
        'Authorization': 'Bearer token',
        'X-Custom': 'value'
      })
    });
  });

  it('should not allow custom headers to override default User-Agent', async () => {
    const customHeaders = {
      'User-Agent': 'custom-agent'
    };

    await get(mockFetch, 'http://example.com', { headers: customHeaders });

    expect(mockFetch).toHaveBeenCalledWith('http://example.com', {
      headers: expect.objectContaining({
        'User-Agent': expect.stringMatching(/ollama-js\/.*/)
      })
    });
  });

  it('should handle empty headers object', async () => {
    await get(mockFetch, 'http://example.com', { headers: {} });

    expect(mockFetch).toHaveBeenCalledWith('http://example.com', {
      headers: expect.objectContaining(defaultHeaders)
    });
  });
});

@AlexanderSlaa
Copy link
Contributor Author

@BruceMacD Added the test and implemented your suggestion.

Copy link
Collaborator

@BruceMacD BruceMacD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this, the tests failed due to unrelated config issues, so I'm going to merge this one through.

@BruceMacD BruceMacD merged commit 5007ce7 into ollama:main Feb 20, 2025
0 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants